add recursive settings and clean up test whitelist #4142
Conversation
| @@ -21,21 +21,10 @@ const DIFF_CONTEXT_SIZE: usize = 3; | |||
|
|
|||
| // A list of files on which we want to skip testing. | |||
| const SKIP_FILE_WHITE_LIST: &[&str] = &[ | |||
| "issue-3253/paths", | |||
There was a problem hiding this comment.
Placing a remaining whitelisted file up in lexicographical order after 'cleaning house'
| "cfg_mod/dir", | ||
| "cfg_mod/bar.rs", | ||
| "cfg_mod/foo.rs", | ||
| "cfg_mod/wasm32.rs", |
There was a problem hiding this comment.
Between adding the recursive config option to the cfg_if/lib.rs files and possibly some changes that have occurred outside the scope of this PR, a good many of these whitelisted file strings don't seem necessary at this time.
There was a problem hiding this comment.
We need to keep these files in the list to make sure that recursive is working correctly. The test must pass even when these files are skipped.
|
Thanks for the PR @bradleypmartin! Will try to take a look at this later today |
| "cfg_mod/dir", | ||
| "cfg_mod/bar.rs", | ||
| "cfg_mod/foo.rs", | ||
| "cfg_mod/wasm32.rs", |
There was a problem hiding this comment.
We need to keep these files in the list to make sure that recursive is working correctly. The test must pass even when these files are skipped.
| @@ -10,6 +10,7 @@ | |||
| //! * `mips64`: [`is_mips64_feature_detected`] | |||
| //! * `powerpc`: [`is_powerpc_feature_detected`] | |||
| //! * `powerpc64`: [`is_powerpc64_feature_detected`] | |||
| // rustfmt-recursive: true | |||
There was a problem hiding this comment.
I think the system/idempotence checks need the inline config options to be at the top of the files as well
There was a problem hiding this comment.
Thanks, @calebcartwright ! Even if the config options can 'survive' being down below, I also like how having them at the top matches with their location in other files already. I've gone ahead and moved them up there.
I'll wait on topecongiro's feedback on the particular files we can take off the whitelist, then make those changes as well (feel free to weigh in there too if you've got strong opinions).
|
Hey @topecongiro , thanks for the feedback! To clarify: it was pretty evident that the following three files were being intentionally ignored to test
...as for the others, though (the It seems like we can probably at least leave out the two Just let me know and I'll fix it up! |
I don't expect we'll need to remove any files from the skip file list. The main integration test suite for rustfmt (usually referenced as the system test suite) works by checking every file individually under the A few of these tests (including the If we were to remove those files from the skiplist then that test would no longer necessarily validate the mod behavior, since rustfmt would have formatted each of those other files individually. |
|
Once again, I appreciate the patient help and context while I sync up here! For the moment I'm reverting the whitelist and I'll take a closer look this evening. I think something I find challenging is that - at first glance - it's difficult to parse exactly what the purpose of some of these files' whitelisting is...especially if existing unit/integration tests pass regardless of whether the files are present in the whitelist or not. ...it's like, for someone looking at the SKIP_FILE list for the first time, the intuitive/'first-order' understanding is that not skipping the files (having them absent from the list) would result in 'more rigorous' testing (i.e. not being skipped). But with both of your feedback and thinking about it some more, it's certainly clear that having a file in an 'ignore/skip' list might in fact convey exactly the testing logic/behavior that you're after...it's just (totally in my opinion) more counterintuitive and takes more time to wrap my head around the context. Thanks again for the help! |
|
No worries, and thanks for the feedback! We want rustfmt to be as easy as possible for folks to contribute to, so it's great to hear about your experience.
The gist of #4113 is essentially that some integration tests were no longer running at all, and the cause of that was due to a misconfiguration in those tests (the absence of the recursive setting). We want to fix those by correcting the configuration used for those tests, and not by removing files from the skip list. There's a pretty comprehensive (and ever-growing) testing suite within rustfmt, but I think it's fair to say we could take another look through the Contributor and testing documentation to make sure things are up to date and clear especially to include some things we know are missing (#4071 (comment)). IMHO that's something best tackled in a separate PR though |
Just to follow up on these points, I think there may still be some confusion on what that skip list is used for. rustfmt only formats the files provided as args, unless the The skip list in many cases is being used so that we don't run rustfmt directly against those individual files like all of the others, so that we can validate those particular files get formatted because they were imported elsewhere and resolved correctly. For example with We want the test to run this, and only this, for the rustfmt --recursive rustfmt-core/rustfmt-lib/tests/source/cfg_mod/mod.rsAnd then validate that all the other files ( If we remove these entries from the skip list, then the test runner will end up running the equivalent of rustfmt rustfmt-core/rustfmt-lib/tests/source/cfg_mod/mod.rs
rustfmt rustfmt-core/rustfmt-lib/tests/source/cfg_mod/bar.rs
rustfmt rustfmt-core/rustfmt-lib/tests/source/cfg_mod/foo.rs
rustfmt rustfmt-core/rustfmt-lib/tests/source/cfg_mod/somewhere.rs
rustfmt rustfmt-core/rustfmt-lib/tests/source/cfg_mod/dir/dir1/dir2/wasm32.rs
rustfmt rustfmt-core/rustfmt-lib/tests/source/cfg_mod/dir/dir1/dir2/wasm32.rs
etc.The end result of the formatting out is the same in both cases, but in the latter case we wouldn't have a way of knowing whether the mod path resolution was done correctly. By adding those other files to the skip list, then the test will fail if the path resolution does ever break. We have tests like these to validate core behavior of rustfmt and to prevent any regressions. Internally rustfmt uses a pinned version of rustc parser to parse the input files, and it's especially important when we upgrade the rustc parser version that rustfmt is still working correctly with those upstream parser changes. The impetus for #4113 was actually such recent rustc upgrade that broke the path resolution within rustfmt. We caught it manually through some other testing, but realized our automated testing for those scenarios (like Anyway, probably way more than you wanted to know 😄 Most of the time the skip list isn't something rustfmt contributors have to deal with, but I hope that helps explain the context. |
|
Hey, no worries on giving more information than I need - anything and everything's appreciated! :) Got kind of busy at work, but this extra info will be immensely helpful when I get some bandwidth free...hopefully at the weekend if not before. Thanks once again! |
|
@calebcartwright Thanks for adding that label and sorry about the wait! Looking at this in earnest right now. Hope you're all enjoying a relaxing Memorial Day weekend (for those in the US)! |
|
Also saw that recent builds were kinda problematic via rust-lang/rust#72232; I'll work from a less recent commit until that gets resolved. EDIT: Sandboxing my changes over at my forked rustfmt: bradleypmartin#1. |
At this point the upstream issue is only impacting the availability of rustfmt via rustup, but you should be able to rebase your changes on master and compile using a recent nightly |
Servo has used this since forever, and it'd be useful to be able to use rustfmt stable there so that we can use the same rustfmt version in both Firefox and Servo. Feel free to close this if there's any reason it shouldn't be done. Closes rust-lang#3368
Previously, non-trivial type aliases in extern blocks were dropped by rustfmt because only the type alias name would be passed to a rewritter. This commit fixes that by passing all type information (generics, bounds, and assignments) to a type alias rewritter, and consolidates `rewrite_type_alias` and `rewrite_associated_type` as one function. Closes rust-lang#4159
* Preserve comments in empty statements Closes rust-lang#4018 * fixup! Preserve comments in empty statements
…ving a struct's commas (rust-lang#4201)
…om/bradleypmartin/rustfmt into bradleypmartin-test-recursive-mode
|
Thanks! |
|
Hey @topecongiro @calebcartwright , thanks a lot again for the patience and help with this one, and I'll try and do a better job of understanding the problem space/scope before I submit my next OSS PR. Have a great day! |
|
No worries @bradleypmartin, thank you for your interest and contribution! |
|
Hey, I think you forgot to close the associated issue |
Close (issue) #4113.
Within the
cfg_ifdirectory, there are some nested files that can be helped out with a recursive config setting and then removed from the test whitelist inrustfmt-core/rustfmt-lib/src/test/mod.rs.In addition to the
cfg_iffiles on the whitelist, there are quite a few others that don't seem to be needed there.The files that remain appear to be whitelisted for reasons outside the scope of the Issue linked above. For example,
issue-3253/pathsnests some files whose tests fail despite having a// rustfmt-recursive: trueconfig setting put into thelib.rsfile in theissue-3253root folders (source/target).