Skip to content

fix(orchestrator): mock starting processes in upgrade unit tests#9528

Merged
pierugo-dfinity merged 14 commits intomasterfrom
pierugo/orchestrator/mock-replica-and-manageboot
Mar 24, 2026
Merged

fix(orchestrator): mock starting processes in upgrade unit tests#9528
pierugo-dfinity merged 14 commits intomasterfrom
pierugo/orchestrator/mock-replica-and-manageboot

Conversation

@pierugo-dfinity
Copy link
Copy Markdown
Contributor

@pierugo-dfinity pierugo-dfinity commented Mar 20, 2026

This PR extends #9521 to also mock the execution of manageboot.sh in an effort to deflake //rs/orchestrator:orchestrator_test. Similarly, we replace all call sites with a trait function that is implemented differently based on an argument of the Upgrade struct.

The PR also turns relevant String/&str to OsString/&OsStr when they are used in places expecting an AsRef<OsStr> to avoid awkward casts like PathBuf.as_path().display().to_string() (and to make more sense conceptually).

…tests (#9521)

This PR deflakes `//rs/orchestrator:orchestrator_test` by mocking the
process management of the orchestrator such that no process is actually
spawned when the upgrade loop supposedly starts the replica.
To do so, this moves the struct `ProcessManager` to a trait that exposes
the public functions and leaves the implementation to
`ProcessManagerImpl`. Now, a `FakeProcessManager` can be used, whose
implementation is keeping track of a boolean mocking whether the process
is running or not.

This was mostly AI-generated, supervised and reviewed by me.
@github-actions github-actions bot added the fix label Mar 20, 2026
@pierugo-dfinity pierugo-dfinity marked this pull request as ready for review March 23, 2026 16:16
@pierugo-dfinity pierugo-dfinity requested a review from a team as a code owner March 23, 2026 16:16
@pierugo-dfinity
Copy link
Copy Markdown
Contributor Author

Now that the location of mangeboot.sh is determined in orchestrator.rs, I am wondering if, for consistency, we should also replace passing down ic_binary_directory to all sub-tasks and instead pass only the location of the binary they are interested in, i.e. ic_binary_directory.join("replica") for Upgrade, ic_binary_directory.join("ic-boundary") for BoundaryNodeManager and ic_binary_directory.join("guestos_tool") for Ipv4Configurator. WDYT?

@pierugo-dfinity pierugo-dfinity added the CI_ALL_BAZEL_TARGETS Runs all bazel targets label Mar 24, 2026
@pierugo-dfinity pierugo-dfinity added this pull request to the merge queue Mar 24, 2026
Merged via the queue into master with commit 8de0763 Mar 24, 2026
38 checks passed
@pierugo-dfinity pierugo-dfinity deleted the pierugo/orchestrator/mock-replica-and-manageboot branch March 24, 2026 15:28
alin-at-dfinity pushed a commit that referenced this pull request Mar 26, 2026
This PR extends #9521 to also mock the execution of `manageboot.sh` in
an effort to deflake `//rs/orchestrator:orchestrator_test`. Similarly,
we replace all call sites with a trait function that is implemented
differently based on an argument of the `Upgrade` struct.

The PR also turns relevant `String`/`&str` to `OsString`/`&OsStr` when
they are used in places expecting an `AsRef<OsStr>` to avoid awkward
casts like `PathBuf.as_path().display().to_string()` (and to make more
sense conceptually).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants