Pass environment variables defined in config.toml

[arxanas]

No description provided.

[sunshowers]

I think this is a great start! Left a few comments.

[arxanas]

@sunshowers can you approve the workflow runs?

[sunshowers]

Just did! Unfortunately each time you update I'll have to re-approve it. If you do another PR with a simple change (like the change to set_env_vars above) I can land that and then it'll be easier for you. Let me know what works.

[arxanas]

@sunshowers I updated this PR with my latest changes. It looks like for the basic test, CargoConfigs is picking up the configs in .cargo/config.toml instead of fixtures/nextest-tests/.cargo/config.toml, which I guess makes sense, but doesn't help for testing. Should I be invoking the test fixture differently? Currently, I'm running

$ cargo local-nt run -- basic

Similarly, I don't see that the target triple was detected for the basic test, although it doesn't seem like there's any tests which depend on that.

If I do set it in .cargo/config.toml instead of inside the fixture config, it causes other problems: since nextest is invoking itself (IIUC?), the outermost invocation successfully picks up the environment variables, which means that the innermost invocation sees them in the environment as if they had been explicitly set by the user. It then thinks that the user meant to explicitly set those environment variables, so it declines to set any environment variables which aren't marked as force. It should actually not observe the variables in the environment and it should set all of them, except for the ones that were previously set in set_env_vars.

[arxanas]

@sunshowers Thanks for the feedback. I believe I addressed all the concerns.

I think the tests are passing, except for the integration tests which try to run cargo nextest run on a project in a different directory (i.e. set --manifest-path). As far as I can tell, those invocations shouldn't pick up the environment variables defined in the fixture's config, because only the current working directory and its ancestors are supposed to be searched:

If, for example, Cargo were invoked in /projects/foo/bar/baz, then the following configuration files would be probed for and unified in this order:

• /projects/foo/bar/baz/.cargo/config.toml
• /projects/foo/bar/.cargo/config.toml
• /projects/foo/.cargo/config.toml
• /projects/.cargo/config.toml
• /.cargo/config.toml
• $CARGO_HOME/config.toml which defaults to:
  • Windows: %USERPROFILE%.cargo\config.toml

...

At present, when being invoked from a workspace, Cargo does not read config files from crates within the workspace. i.e. if a workspace has two crates in it, named /projects/foo/bar/baz/mylib and /projects/foo/bar/baz/mybin, and there are Cargo configs at /projects/foo/bar/baz/mylib/.cargo/config.toml and /projects/foo/bar/baz/mybin/.cargo/config.toml, Cargo does not read those configuration files if it is invoked from the workspace root (/projects/foo/bar/baz/).

From https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure.

I tried changing it so that the cargo search path uses the provided --manifest-path, and it works for most test cases. (It currently doesn't work when testing from an archive; not sure why.) Let me know what you think we should do about how we read the cargo config for these test cases.

[sunshowers]

Thanks! Just saw this, apparently I missed your last message. Sorry about that!

As far as I can tell, those invocations shouldn't pick up the environment variables defined in the fixture's config, because only the current working directory and its ancestors are supposed to be searched:

Yes, that's right. So if you can find a way to flag those runs (perhaps by setting an environment variable like NOT_IN_WORKSPACE_CWD or something), then in the test itself you can branch on that env var.

I tried changing it so that the cargo search path uses the provided --manifest-path, and it works for most test cases

I think matching cargo's behavior is the right thing to do here, and cargo's behavior is that --manifest-path isn't considered for cargo search paths. So I'd suggest instead:

1. Going back to the old behavior of searching for configs in the cwd.
2. Flagging test runs with --manifest-path with the env var as above, and/or replacing some of those --manifest-path invocations with cding into the directory. (With nextest, each test runs in its own process, so you can even chdir the current process without worrying about trampling over other tests.)

[codecov]

Codecov Report

Merging #534 (b88612d) into main (c913799) will increase coverage by 0.33%.
The diff coverage is 95.74%.

❗ Current head b88612d differs from pull request most recent head 5ae48f1. Consider uploading reports for the commit 5ae48f1 to get more accurate results

@@            Coverage Diff             @@
##             main     #534      +/-   ##
==========================================
+ Coverage   76.79%   77.13%   +0.33%     
==========================================
  Files          49       49              
  Lines       11426    11596     +170     
==========================================
+ Hits         8775     8944     +169     
- Misses       2651     2652       +1     

Impacted Files	Coverage Δ
nextest-runner/src/reporter/aggregator.rs	35.89% <0.00%> (ø)
nextest-runner/src/list/test_list.rs	92.10% <91.66%> (-0.13%)	⬇️
cargo-nextest/src/dispatch.rs	82.07% <100.00%> (+0.06%)	⬆️
cargo-nextest/src/tests_integration/fixtures.rs	93.59% <100.00%> (+0.09%)	⬆️
nextest-metadata/src/test_list.rs	33.95% <100.00%> (+0.41%)	⬆️
nextest-runner/src/cargo_config.rs	93.71% <100.00%> (+2.15%)	⬆️
nextest-runner/src/reporter.rs	50.93% <100.00%> (ø)
nextest-runner/src/runner.rs	83.22% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sunshowers]

Looks good, just a couple of comments that I'll address myself before landing.

[sunshowers]

I've done a number of updates and fixes myself, but the one I'm requesting you do is a more significant change.

[arxanas]

I added commit 604e91b with just my changes to fix the tests. Let me know if you want me to squash the two commits together.

[arxanas]

Re CI:

• The retry test case on macOS doesn't reproduce for me with cargo local-nt run --no-default-features. Is it flaky?
• It looks like there's a path-related test failing under Windows:

nextest/fixtures/nextest-tests/tests/basic.rs

Lines 165 to 172 in 27f7f8d

	let config_workspace_dir = PathBuf::from(
	std::env::var("CONFIG_WORKSPACE_DIR")
	.expect("CONFIG_WORKSPACE_DIR should be present, defined in [env]"),
	);
	assert_eq!(
	config_workspace_dir,
	PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap())
	);

Do you understand why it's failing? (Unfortunately, I don't have a Windows machine to test on.)

[sunshowers]

The macOS test is definitely flaky, I've been meaning to get to the bottom of it for a while.

The Windows issues are pretty fun, I know what's going on. The rest of the PR looks really good. Let me see if I can fix it up and land it shortly.

[sunshowers]

Looks fantastic, thanks for working through everything.

[sunshowers]

I'm going to get this out in the next couple of days. Thanks again!

[sunshowers]

BTW I ended up redoing the EnvironmentMap construction to fix some lingering issues including case-insensitivity on Windows.