Add test server modes to improve testing client library edge cases#68
Closed
Add test server modes to improve testing client library edge cases#68
Conversation
DRMacIver
added a commit
to hegeldev/hegel-rust
that referenced
this pull request
Mar 26, 2026
Tests server error paths using the test server's error simulation modes: - stop_test_on_start_span: covers StopTest during span operations - health_check_failure: covers health check failure panic - server_error_in_results: covers server error panic - flaky_replay: covers FlakyReplay error handling Requires hegel-core with new test modes (hegeldev/hegel-core#68). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DRMacIver
force-pushed
the
add-test-modes-for-sdk-coverage
branch
from
a529f8c to
100e02f
Compare
DRMacIver
added a commit
to hegeldev/hegel-rust
that referenced
this pull request
Mar 26, 2026
Tests use HEGEL_PROTOCOL_TEST_MODE to exercise server error paths. Currently hang due to server process lifecycle issue in TempRustProject subprocesses — the HegelSession static doesn't clean up on panic. Depends on hegeldev/hegel-core#68 for new test modes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DRMacIver
force-pushed
the
add-test-modes-for-sdk-coverage
branch
8 times, most recently
from
2674796 to
991f01c
Compare
DRMacIver
added a commit
to hegeldev/hegel-rust
that referenced
this pull request
Mar 26, 2026
Tests that use new test modes from hegeldev/hegel-core#68 (not yet released) are skipped in CI where only the released hegel-core is available. The requires_new_modes!() macro checks if the mode exists before running the test. Also: fall back to system hegel if local venv doesn't exist. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DRMacIver
force-pushed
the
add-test-modes-for-sdk-coverage
branch
from
991f01c to
636cbd0
Compare
DRMacIver
changed the title
DRMacIver
force-pushed
the
add-test-modes-for-sdk-coverage
branch
2 times, most recently
from
193c85a to
ff0c44f
Compare
New modes: stop_test_on_start_span, server_crash, health_check_failure, server_error_in_results, flaky_replay. Plus conformance test classes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DRMacIver
force-pushed
the
add-test-modes-for-sdk-coverage
branch
from
ff0c44f to
935832b
Compare
- Add test_pool_commands_handled_before_start_span to exercise the new_pool/pool_add/generate branches in _handle_commands_until - Split flaky_replay test into two: one that sends mark_complete (exercises the try success path) and one that doesn't (exercises the except timeout path) - Fix TestServerCrash to expect ConnectionError with pytest.raises Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These modules are already imported at the top of the file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DRMacIver
commented
Mar 27, 2026
| _send_test_done(test_channel) | ||
|
|
||
|
|
||
| def _mode_failed_no_reason( |
Member
Author
There was a problem hiding this comment.
Registering that I find this mode a little suss and maybe we should just assume this failure doesn't happen, but decided on balance that it's not a bad policy to have clients be a bit postel's law about servers, especially as we want to open up the protocol later.
Liam-DeVoe
reviewed
Mar 27, 2026
Comment on lines
+222
to
+226
| assert ( | ||
| "health check" in result.stderr.lower() | ||
| or "Health check" in result.stderr | ||
| ) | ||
|
|
Member
There was a problem hiding this comment.
I don't understand why we need two cases here. If we do then we should just check health check in s.lower(), because that subsumes Health check in s
Comment on lines
+202
to
+220
| class HealthCheckFailureConformance(ErrorHandlingConformance): | ||
| """Conformance test for health check failure reported in test_done.""" | ||
|
|
||
| test_mode = "health_check_failure" | ||
|
|
||
| def run(self, params: dict[str, Any]) -> None: | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".jsonl") as f: | ||
| input_json = json.dumps(params) | ||
| result = subprocess.run( | ||
| [str(self.binary), input_json], | ||
| env={ | ||
| **os.environ, | ||
| "CONFORMANCE_METRICS_FILE": f.name, | ||
| "CONFORMANCE_TEST_CASES": str(self.test_cases), | ||
| **self.extra_env(), | ||
| }, | ||
| capture_output=True, | ||
| text=True, | ||
| ) |
Member
There was a problem hiding this comment.
we can't be reimplementing ConformanceTest.run in subclasses. Why was this necessary? What hooks do we need to provide on ConformanceTest to make this not necesary?
Member
Author
|
Closing this as part of the general ditching the slop that came out of the previous iteration of 100% coverage work for rust. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New test server modes for SDK error handling coverage:
stop_test_on_start_span: StopTest during span operationsserver_crash: Process exit mid-testhealth_check_failure: Health check failure in test_doneserver_error_in_results: Server error in test_doneflaky_replay: FlakyReplay error on generateThis is as part of the ongoing work to get rust coverage to 100% - there are a lot of paths that are hard to test without getting specific errors from the server, and this just gets the server to trigger them directly.