feat(huntsman): Add the end-to-end integration-test driver. - #379
Conversation
WalkthroughAdds a workspace-integrated Huntsman E2E crate with shared job types and a singleton ChangesHuntsman E2E test harness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HuntsmanE2EScenario
participant SpiderTestDriver
participant SpiderClient
participant SpiderDeployment
HuntsmanE2EScenario->>SpiderTestDriver: run or run_exclusive(JobSubmission)
SpiderTestDriver->>SpiderClient: submit and start job
SpiderClient->>SpiderDeployment: execute job
SpiderTestDriver->>SpiderClient: poll job state and outputs
HuntsmanE2EScenario->>SpiderTestDriver: perform failure injection callback
SpiderClient-->>SpiderTestDriver: terminal state
SpiderTestDriver-->>HuntsmanE2EScenario: invoke outcome assertion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Let's merge #377 first and integrate NN-testing into this crate after that.
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if the default value is zero. |
There was a problem hiding this comment.
For most libraries/software I have used, 0 for concurrency setting usually means using the number of CPUs as the concurrency level.
There was a problem hiding this comment.
I should document this clearly: the default value is a compile-time constant which should never be 0.
| const DEFAULT_CONCURRENCY: usize = 8; | ||
|
|
||
| match std::env::var(CONCURRENCY_ENV_VAR) { | ||
| Ok(value) => value.parse::<NonZeroUsize>().with_context(|| { |
There was a problem hiding this comment.
We should guard against negative values and a crazily-large value.
There was a problem hiding this comment.
- Parsing a negative number should fail 'cuz the given type is unsigned.
- I don't think it's necessary for a tester.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/huntsman/e2e/src/types.rs (2)
8-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving
DebugforTerminationResult.Without
Debug, assertion failures that include aTerminationResultvalue will produce unhelpful error messages. DerivingDebugis low-cost and improves test diagnostics.♻️ Proposed refactor
+#[derive(Debug)] pub enum TerminationResult {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/huntsman/e2e/src/types.rs` around lines 8 - 17, Derive the standard Debug trait for the TerminationResult enum so assertion failures can display its values; add Debug to the enum’s derives and ensure all contained types, including TaskOutput, support Debug.
20-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving
DebugforJobSubmission.Same rationale — deriving
Debugaids diagnostics when submissions are logged or included in assertion failures.♻️ Proposed refactor
+#[derive(Debug)] pub struct JobSubmission {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/huntsman/e2e/src/types.rs` around lines 20 - 30, Derive the Rust Debug trait for the JobSubmission struct so job submissions can be formatted in diagnostics, logs, and assertion failures; update its derive attributes while preserving the existing fields and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/huntsman/e2e/src/test_driver.rs`:
- Line 160: Configure an explicit connection timeout before creating the client
in the test initialization flow around SpiderClient::connect, preferably in the
shared ConnectionPool::connect helper or by setting connect_timeout on the
Endpoint. Ensure unreachable Spider endpoints fail within a bounded duration
instead of allowing init to hang indefinitely.
- Around line 58-63: Acquire the concurrency permit before obtaining the client
read lock in the shared-scenario path. In the relevant test driver method, move
the concurrency_limiter.acquire() call and its context handling ahead of
driver.client.read().await so scenarios waiting for capacity do not hold the
read lock and can allow run_exclusive to acquire the write lock.
---
Nitpick comments:
In `@tests/huntsman/e2e/src/types.rs`:
- Around line 8-17: Derive the standard Debug trait for the TerminationResult
enum so assertion failures can display its values; add Debug to the enum’s
derives and ensure all contained types, including TaskOutput, support Debug.
- Around line 20-30: Derive the Rust Debug trait for the JobSubmission struct so
job submissions can be formatted in diagnostics, logs, and assertion failures;
update its derive attributes while preserving the existing fields and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1b39dbc-4a85-4e80-86eb-36d2c35f045e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomltests/huntsman/e2e/Cargo.tomltests/huntsman/e2e/src/lib.rstests/huntsman/e2e/src/test_driver.rstests/huntsman/e2e/src/types.rs
| let client_guard = driver.client.read().await; | ||
| let _permit = driver | ||
| .concurrency_limiter | ||
| .acquire() | ||
| .await | ||
| .context("concurrency limiter closed")?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Acquire the concurrency permit before the read lock to prevent exclusive-scenario starvation.
The read lock on client is acquired at Line 58 before the semaphore permit at Line 59. If all permits are exhausted, a new shared scenario holds a read lock while waiting for a permit. tokio's RwLock does not guarantee write-lock fairness, so exclusive scenarios (which need a write lock via run_exclusive) can be starved indefinitely as long as new shared scenarios keep arriving and acquiring read locks before blocking on the semaphore.
Swapping the order ensures that waiting for a permit does not block exclusive scenarios.
🔒 Proposed fix
let driver = Self::instance().await?;
- let client_guard = driver.client.read().await;
let _permit = driver
.concurrency_limiter
.acquire()
.await
.context("concurrency limiter closed")?;
+ let client_guard = driver.client.read().await;
let client = client_guard.clone();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let client_guard = driver.client.read().await; | |
| let _permit = driver | |
| .concurrency_limiter | |
| .acquire() | |
| .await | |
| .context("concurrency limiter closed")?; | |
| let _permit = driver | |
| .concurrency_limiter | |
| .acquire() | |
| .await | |
| .context("concurrency limiter closed")?; | |
| let client_guard = driver.client.read().await; | |
| let client = client_guard.clone(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/huntsman/e2e/src/test_driver.rs` around lines 58 - 63, Acquire the
concurrency permit before obtaining the client read lock in the shared-scenario
path. In the relevant test driver method, move the concurrency_limiter.acquire()
call and its context handling ahead of driver.client.read().await so scenarios
waiting for capacity do not hold the read lock and can allow run_exclusive to
acquire the write lock.
2 participants
Description
This PR adds the end-to-end integration-test driver as a new
e2ecrate undertests/huntsman/. The driver wrapsspider-client'sSpiderClientin a process-wide singleton with the concurrency control that end-to-end scenarios share, and exposes a small surface for driving a job (task graph + inputs) through a live Spider deployment and asserting its terminal outcome.This PR adds the driver library only; the concrete test scenarios that use it are added in a follow-up.
Note
This PR does not add test scenarios. The
e2ecrate builds and lints as a library, and itstests/directory is reserved for the scenarios added later.New
e2ecrate (tests/huntsman/e2e/)tests/huntsman/e2e/as a new workspace member. The crate is namede2e: itssrc/is the driver library, and itstests/directory is reserved for the scenario suite.spider-clientandspider-corefor the gRPC and core types,tokio(sync/timefor the concurrency primitives and polling),tonic(to build the endpoint), andanyhowfor the high-level error surface.Scenario types (
src/types.rs)TerminationResult— the terminal outcome a scenario converges to:Success(Vec<TaskOutput>)carrying the job's task outputs,Failure(String)carrying the reported error message, orCancelled.JobSubmission— a single job to drive: an external resource-group id (String), theTaskGraph, and the job-levelinputs(Vec<TaskInput>).The driver (
src/test_driver.rs)SpiderTestDriveris a lazily initialized, process-wide singleton (tokio::sync::OnceCell). On first access it reads the Spider endpoint fromSPIDER_ENDPOINTand the concurrency fromSPIDER_CONCURRENCY(defaulting to8), then connects aSpiderClientpool sized to that concurrency. It holds the client behind aRwLock, aSemaphoresized to the concurrency, and aMutex<HashMap<String, ResourceGroupId>>for resource-group resolution.SpiderTestDriver::runis the non-exclusive entry point: it acquires the client's shared read lock and a concurrency permit from the semaphore, then drives the scenario with no failure injection. Multiplerunscenarios execute concurrently up to the configured limit.SpiderTestDriver::run_exclusiveis the exclusive entry point: it acquires the client's write lock so no other scenario runs concurrently, and drives the scenario with a caller-supplied failure-injection coroutine, which is what a failure-recovery scenario uses to perturb a running job.run, the permit) is held for the entire scenario; the client is cloned under the lock and passed by value into the scenario runner.ResourceGroupId, registering it through the client on first use (with an empty password) and caching the result under the table lock, so subsequent submissions reuse the same id.run_scenariois the shared core: it submits and starts the job, then concurrently runs the failure-injection coroutine while polling the job to a terminal state, bounding the whole race bytimeout, and finally hands theTerminationResultto the caller's outcome-assertion coroutine.submit_and_start_jobperforms both the submit and the start of the job through the client.poll_until_terminalpolls the job state every 10ms viaget_job_state. On a terminal state it builds the correspondingTerminationResult:Successretrieves the job outputs,Failureretrieves the job error message, andCancelledcarries no payload.Checklist
breaking change.
Validation performed
Summary by CodeRabbit