Skip to content
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

Move unit tests into separate files unconfigured during normal build #61097

Closed
petrochenkov opened this issue May 23, 2019 · 13 comments · Fixed by #63207
Closed

Move unit tests into separate files unconfigured during normal build #61097

petrochenkov opened this issue May 23, 2019 · 13 comments · Fixed by #63207
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@petrochenkov
Copy link
Contributor

One of the most annoying parts of compiler refactorings is suddenly discovering that after a refactoring change some code in unit tests stops building.

Compiler unit tests are mostly old, many of them are not really "unit" and need to be migrated to regular ui testing or removed, but that's not the point of this issue.
The point of this issue is that after changing such test you have to restart the bootstrapping process from the crate that unit test belongs to (e.g. touched libsyntax -> rebuild everything).

How to fight this problem?
Exclude unit tests from the normal build!

Files included from under cfg(FALSE) are not considered crate dependencies from build system point of view (not included into dep files).
So, unit test modules included in the next way

#[cfg(test)]
mod tests;

won't cause rebuilds after changing code in them, they will be rebuilt only as a part of unit test run (so they won't participate in bootstrap).

Steps:

  • Standardize on the test module naming (currently mod test - 31 instances, mod tests - 183 instances, excluding src/tools and src/test subdirectories).
  • Outline test modules into separate files mod tests { ... } -> #[cfg(test)] mod tests;.
  • Running unit tests for my_crate: ./x.py test --stage 0 src/my_crate.
@petrochenkov petrochenkov added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-testsuite Area: The testsuite used to check the correctness of rustc E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels May 23, 2019
@chansuke
Copy link
Contributor

I'd like to work on this.

@petrochenkov
Copy link
Contributor Author

Standardize on the test module naming (currently mod test - 31 instances, mod tests - 183 instances...

Test modules beside test/tests:

mod __test
mod boxed_test
mod test_json_encode_decode
mod unindent_tests
mod test_map
mod test_set
mod sync_tests
mod dynamic_tests
mod parser_testing
mod test_snippet

@hellow554
Copy link
Contributor

hellow554 commented May 25, 2019

I'd like to work on this.

You could do @rustbot claim to let everyone know that you have claimed the issue :)

@chansuke
Copy link
Contributor

@hellow554 Thank you

@chansuke
Copy link
Contributor

@petrochenkov Hi. I have a question:) I found pub mod tests in src/test/codegen/fastcall-inregs.rs.
Can I just touch src/test/codegen/tests.rs and just move them

    // CHECK: @f1(i32 inreg %arg0, i32 inreg %arg1, i32 %arg2)
    #[no_mangle]
    pub extern "fastcall" fn f1(_: i32, _: i32, _: i32) {}

    // CHECK: @f2(i32* inreg %arg0, i32* inreg %arg1, i32* %arg2)
    #[no_mangle]
    pub extern "fastcall" fn f2(_: *const i32, _: *const i32, _: *const i32) {}

    // CHECK: @f3(float %arg0, i32 inreg %arg1, i32 inreg %arg2, i32 %arg3)
    #[no_mangle]
    pub extern "fastcall" fn f3(_: f32, _: i32, _: i32, _: i32) {}

    // CHECK: @f4(i32 inreg %arg0, float %arg1, i32 inreg %arg2, i32 %arg3)
    #[no_mangle]
    pub extern "fastcall" fn f4(_: i32, _: f32, _: i32, _: i32) {}

    // CHECK: @f5(i64 %arg0, i32 %arg1)
    #[no_mangle]
    pub extern "fastcall" fn f5(_: i64, _: i32) {}

    // CHECK: @f6(i1 inreg zeroext %arg0, i32 inreg %arg1, i32 %arg2)
    #[no_mangle]
    pub extern "fastcall" fn f6(_: bool, _: i32, _: i32) {}

into that file(src/test/codegen/tests.rs)?

@petrochenkov
Copy link
Contributor Author

@chansuke
Anything inside src/test can be left unchanged, that's a directory with integration tests and the rebuild problem doesn't apply to it.
(Also, git submodule directories in src/tools and src/doc are also out of scope.)

@chansuke
Copy link
Contributor

@petrochenkov Thanks.

@Mark-Simulacrum
Copy link
Member

It'd be great to add a tidy lint against mod test { as a simple heuristic that we don't accidentally re-introduce these into the compiler.

@Mark-Simulacrum
Copy link
Member

Just noticed that we'll probably want to update tidy's pal lint to whatever name we standardize to:

if let Some(mod_tests_idx) = contents.find("mod tests") {

@petrochenkov
Copy link
Contributor Author

@chansuke
Any news?
This is a relatively large work (~200 files), even if it's largely mechanical, so it's not necessary to do it in one PR, it can be landed in whatever quantities it's more convenient to implement.

@chansuke
Copy link
Contributor

@petrochenkov Hey, sorry for the late reply. I've finished like half of the modules and still working on this issue. Yes, I will send [WIP] PR.

bors added a commit that referenced this issue Jun 16, 2019
Separate unit tests

I'm working on #61097.

About half of the modules are done but dozens of modules are still remaining. I will rebase and squash the commits later.
@petrochenkov
Copy link
Contributor Author

The tidy check is implemented in #62996.

Centril added a commit to Centril/rust that referenced this issue Jul 27, 2019
tidy: Add a check for inline unit tests

As described in rust-lang#61097.

There's a large whitelist right now, because in many crates the tests are not outlined yet.
~This PR only outlines tests in one crate (`rustc_lexer`) as an example.~

r? @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this issue Jul 27, 2019
tidy: Add a check for inline unit tests

As described in rust-lang#61097.

There's a large whitelist right now, because in many crates the tests are not outlined yet.
~This PR only outlines tests in one crate (`rustc_lexer`) as an example.~

r? @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this issue Jul 28, 2019
tidy: Add a check for inline unit tests

As described in rust-lang#61097.

There's a large whitelist right now, because in many crates the tests are not outlined yet.
~This PR only outlines tests in one crate (`rustc_lexer`) as an example.~

r? @Mark-Simulacrum
@petrochenkov
Copy link
Contributor Author

The rest of the work is done in #63207.

bors added a commit that referenced this issue Aug 2, 2019
Unconfigure compiler unit test files during normal build

I haven't touched libstd though, it had a lot of tests and I'm not sure the people maintaining it want this.

Closes #61097
r? @Mark-Simulacrum
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants