Skip to content

Add a warning for unknown default profiles#747

Merged
sunshowers merged 1 commit into
nextest-rs:mainfrom
marcelogomez:warn-default-profile
Jan 1, 2023
Merged

Add a warning for unknown default profiles#747
sunshowers merged 1 commit into
nextest-rs:mainfrom
marcelogomez:warn-default-profile

Conversation

@marcelogomez

Copy link
Copy Markdown
Contributor

Summary

This PR addresses #388:

  • Introduces NextestConfigImpl::DEFAULT_PROFILES, a const containing all the nextest defined profiles (at the moment this only includes default and default-miri)
  • Looks through all profiles in a project's config for profiles with the default- prefix that are not in NextestConfigImpl::DEFAULT_PROFILES and produces a warning for each of them

Note that I ended up doing this in nextest-runner/src/config.rs instead of in cargo-nextest since AFAICT this is where a lot of the early validation of config files is being done and similar warnings were already being emitted there. Happy to move if you still feel the binary (e.g. somewhere in nextest-runner/src/dispatch.rs?) is a better place for this.

Test Plan

  • Ran cargo xfmt

  • Ran cargo nextest run --all-features. Saw integration-tests failures but they seem unrelated to my changes and they repro on base rev, hopefully CI will tell me if they're legit.

  • Set up a dummy library project with some profiles that should and some that shouldn't trigger this warning.

[profile.default-unknown]
fail-fast = false

[profile.default-yet-another-unknown]
fail-fast = false

[profile.not-default-unknown]
fail-fast = true

Then ran the tests with my changes, verified that only the expected profiles trigger the warning

➜  nextest-test git:(main) ✗ nt.local nextest run
warning: found unknown profiles in the reserved default- namespace in .config/nextest.toml:
warning:   default-unknown
warning:   default-yet-another-unknown
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
    Starting 2 tests across 1 binaries
        PASS [   0.006s] nextest-test tests::test_bar
        PASS [   0.006s] nextest-test tests::test_foo
------------
     Summary [   0.009s] 2 tests run: 2 passed, 0 skipped

Renamed all profiles to no longer have the default- prefix, verified that no warnings are triggered

➜  nextest-test git:(main) ✗ nt.local nextest run
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
    Starting 2 tests across 1 binaries
        PASS [   0.006s] nextest-test tests::test_bar
        PASS [   0.006s] nextest-test tests::test_foo
------------
     Summary [   0.009s] 2 tests run: 2 passed, 0 skipped

@codecov

codecov Bot commented Dec 30, 2022

Copy link
Copy Markdown

Codecov Report

Merging #747 (5975d97) into main (922a9bb) will increase coverage by 0.02%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##             main     #747      +/-   ##
==========================================
+ Coverage   76.88%   76.90%   +0.02%     
==========================================
  Files          57       57              
  Lines       12623    12640      +17     
==========================================
+ Hits         9705     9721      +16     
- Misses       2918     2919       +1     
Impacted Files Coverage Δ
nextest-runner/src/config.rs 91.89% <56.25%> (+0.02%) ⬆️

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

@sunshowers

Copy link
Copy Markdown
Member

Looks good, thank you!

@sunshowers sunshowers merged commit dd89723 into nextest-rs:main Jan 1, 2023
@FrankReh

Copy link
Copy Markdown

Should #388 be closed?

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.

3 participants