-
Notifications
You must be signed in to change notification settings - Fork 738
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
Test refactor #1019
Test refactor #1019
Conversation
Removes the `test_` prefix from the files and renames the `test` directory to `tests`.
These depend on the nix crate, but that isn't specified as a (development) dependencies. So these benchmarks can't even be run currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I find smaller test files easier to manage, but I'm fine going this route if you want.
The old setup compiled everything in the test directory into a single executable. But your PR results in several different executables. I find it harder to understand the output of "cargo test" when there are several executables in the tests directory. |
@asomers this is the idiomatic way to do it w/ rust & cargo now (mio predates all this). could you elaborate more on what you find harder to understand? |
@asomers I agree that the multiple lists of tests vs 1 single list of tests is somewhat harder to read, but as @carllerche mentioned it's the way to do it in Rust now. It's also the reason why I merged some of the files to reduce the number of lists of tests. |
For one thing, there's a lot more output this way. For another, it's hard to see how many test cases there are in total. And finally, it's harder to see the difference between tests, lib tests, and doc tests in the output. |
@asomers I agree. We can reduce the number of files and thus binaries and sections of tests to make the output a bit more manageable. |
What's the conclusion? |
I've looking making the output more usable and found some issues: rust-lang/cargo#4324, rust-lang/cargo#2832. Basically other people have the same problem but no one improved the situation yet. But I believe this is still the way to go. |
@asomers You ok to proceed? |
@Thomasdezeeuw how do you feel about moving files into a subdirectory? That would still keep the files separate but reduce the number of binaries. The file structure would look like this:
This is the direction I'm moving for my crates, to minimize the number of binaries. It still allows separate test binaries when that's really called for. For example, nix has a few dedicated binaries for tests that don't use the standard test harness, or that must run in their own process for some other reason. |
@asomers I'm not disagreeing that there are issues w/ the idiomatic structure. My personal take is that I don't want to go against the grain here. I'd like mio to just follow idiomatic cargo structure and any discussion about alt layouts / improvements taken up w/ cargo. |
@asomers files two layers deep, e.g. |
fwiw, i've always wished we could add dirs to tests... probably another thing to bring up w/ cargo. |
Perhaps |
@asomers I've experimented with your idea in #1030, only look at the last commit. |
Following some experimentation in #1030, I'm merging this with the indent to move to the structure @asomers laid out in #1019 (comment), once the rustfmt bugs have been resolved (see #1030). |
Step one refactoring the tests.
This
test_
prefix from the test files,test
directory totests
, andThe easiest way to review is to do it per commit as I tried to make small(ish) logical changes per commit. But combined they become rather big.
No change were functionally changed or removed.
Updates #995.