Skip to content

[WIP] Fix code as per review comments in PR 690#703

Closed
krishnakumar4a4 with Copilot wants to merge 9 commits into
mainfrom
copilot/fix-code-comments
Closed

[WIP] Fix code as per review comments in PR 690#703
krishnakumar4a4 with Copilot wants to merge 9 commits into
mainfrom
copilot/fix-code-comments

Conversation

Copilot AI commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Address review comments from PR #690 review (pullrequestreview-4705206556):

  • Fix mutex poisoning cascade: replace TEST_MUTEX.lock().unwrap() with unwrap_or_else(|e| e.into_inner()) (9 sites in package_task.rs)
  • tests.rs ~L696: assert --locked/Features are forwarded into metadata provider call in setup helper(s)
  • tests.rs ~L1509: add test that mutates harness.features and asserts the cargo-rustc probe still succeeds (feature forwarding)
  • Validate with clippy/tests

krishnakumar4a4 and others added 8 commits July 15, 2026 18:24
Introduce BuildTaskRunner and PackageTaskRunner with #[automock] to
decouple BuildAction orchestration tests from the full provider stack.
BuildAction tests now mock runners instead of every individual provider,
reducing tests.rs from ~3400 lines to ~700 lines while maintaining
equivalent coverage through dedicated unit tests in build_task.rs and
package_task.rs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
Group top-level orchestration tests into three submodules by scenario type:
- standalone_driver_project: single-package driver builds
- driver_workspace: multi-package Cargo workspace builds
- emulated_workspace: filesystem-based emulated workspace builds

No test logic changes; only structural reorganization for clarity.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
- Rename TestBuildAction to BuildActionHarness for consistency with
  PackageTaskHarness naming convention in the task layer.
- Shorten test function names since submodule names already provide the
  'given' context (e.g. standalone_driver_project::run_skips_probe...).
- Rename test_build_action parameter to harness throughout.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
BuildTaskRunner::run previously collected the entire cargo message stream
into a Vec, which forced every message to be parsed and allocated up front
and prevented get_target_dir_from_output from short-circuiting once the
matching cdylib artifact is found.

Return Box<dyn Iterator<..>> instead so the runner forwards BuildTask::run's
lazy stream while still being mockable (mockall cannot mock an opaque
impl Trait return, but can mock a boxed trait object). BuildTask::run gains
+ use<> precise capturing so its owned, 'static iterator can move out of the
temporary BuildTask and be boxed.

Addresses PR review comment r3581534373.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
Copilot AI requested review from Copilot and removed request for Copilot July 15, 2026 15:26
@krishnakumar4a4

Copy link
Copy Markdown
Contributor

A copilot bug created this PR instead of committing changes to original PR. Closing this one.

Copilot AI requested a review from krishnakumar4a4 July 15, 2026 15:27
Copilot stopped work on behalf of krishnakumar4a4 due to an error July 15, 2026 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants