enable filtered test file execution

[PSeitz]

trafficstars

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

[calebcartwright]

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]

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.

[ytmimi]

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]

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]

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]

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