Skip to content

enable filtered test file execution #5265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Mar 12, 2022

e.g. to execute all files containing 5250
TEST_FILE=5250 cargo test system

@PSeitz PSeitz mentioned this pull request Mar 12, 2022
@calebcartwright
Copy link
Member

calebcartwright commented Mar 16, 2022

Thank you for the PR. I'm not inherently opposed to supporting more targeted testing for system tests, though to be honest I'm not quite sure what the motivation is.

Typically we'll just run cargo run --bin rustfmt -- /file/i/care/about.rs (or invoke the binary from the target directory) and this works great, both for general purpose testing and certainly for idempotence tests. As such I'm curious where you see the benefits of this coming into play, I presume system tests being the only relevant target. Is it perhaps related to ergonomics or test run time, or something else?

@PSeitz
Copy link
Contributor Author

PSeitz commented May 2, 2022

I tried to use cargo run --bin rustfmt -- tests/source/issue-5260.rs, but this doesn't seem to work since the formatter seems to run with different settings:

error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see max_width option), found: 107)

I use TEST_FILE=5250 to just execute a single test, but also print some debug info. Since I'm not familiar with the codebase, this helps me to understand what path the code takes.

e.g. to execute all files containing 5250
TEST_FILE=5250 cargo test system
@ytmimi
Copy link
Contributor

ytmimi commented May 2, 2022

As such I'm curious where you see the benefits of this coming into play, I presume system tests being the only relevant target. Is it perhaps related to ergonomics or test run time, or something else?

@calebcartwright, I think the main benefit would be running the test files with the correct configuration after //rustfmt comments have been parsed. It's maybe just a little more convenient than specifying the configs on the command line, which is what I do when running cargo run --bin rustfmt -- --check --config=...

Granted if someone is unfamiliar with the test suite they might not be aware that we're parsing the //rustfmt comments for configuration values, and that might lead to confusion when rustfmt (with default configs) produces different formatting than the test. @PSeitz is this the issue that you're running into? If so I think there's definitely an argument to be made for being more explicit.

@PSeitz I don't think tests/source/issue-5260.rs exists on master, but I believe the file should be formatted even if you're getting a warning letting you know that some lines are too long. As mentioned above, I'll usually pass the --check flag when executing rustfmt just so I know I'm not overwriting the original file, and then I'll inspect the diff to make sure the changes look alright, or just ensure that there's no diff if I didn't expect anything to change.

Since I'm not familiar with the codebase, this helps me to understand what path the code takes.

If it helps, here's my typical workflow when fixing bugs or implementing features:

  1. create a new file in the root directory with a minimal code snippet. The snippet almost always comes from the issue and I'll name it issue_<number>.rs
  2. run cargo run --bin rustfmt -- --check issue_<number>.rs (optionally passing along --config= if I need to pass configs. Optionally set the RUSTFMT_LOG env var to RUSTFMT_LOG=rustfmt=DEBUG to get some debug output, which will hopefully gives you a general sense of what code is being executed and where you can begin your investigation if you aren't as familiar with the code base.
  3. read through the code and add println! or debug! liberally to anything I think could help me figure out what's going on. I pretty much repeat 2) and 3) until --check either doesn't produce a diff anymore or I get the diff I'm expecting.
  4. run cargo test to make sure my fix didn't break anything else.
  5. turn issue_<number>.rs into a test case.

Not the most sophisticated, but it's gotten me pretty far :)

@calebcartwright
Copy link
Member

Gotcha. We've got an additional special mechanism for config options in system/idempotence test files that we intentionally don't pick up as part of rustfmt itself.

Will think on this a bit more, but I can see the potential utility now

@PSeitz
Copy link
Contributor Author

PSeitz commented May 9, 2022

Granted if someone is unfamiliar with the test suite they might not be aware that we're parsing the //rustfmt comments for configuration values, and that might lead to confusion when rustfmt (with default configs) produces different formatting than the test. @PSeitz is this the issue that you're running into? If so I think there's definitely an argument to be made for being more explicit.

Yes, overall I think there are too many manual steps in comparison to TEST_FILE=5250 cargo test system.

  1. Convert the config to cli-parameters
  2. add check option (if you forget it, it may mess up the uncommited tests)
  3. Check if the output matches the expected test result (Comparing text manually doesn't really work well for me)

Merging this and documenting it in CONTRUBUTING.md imo would make the start easier for contributors. Even better would be cargo test 5250, since this is even closer to the normal workflow, but that would require more work.

@calebcartwright
Copy link
Member

Agreed, you've sold me on the need for something like this, I just wish we could find a way to make it work in a manner that's more consistent/fluid with how tests are filtered elsewhere via the more standard libtest models.

Will take a look at the approach over the next few days but anticipate we'll move forward in some shape or another

@ytmimi ytmimi added the p-low label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants