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

Test refactor continued #1030

Closed
wants to merge 15 commits into from

Conversation

Thomasdezeeuw
Copy link
Collaborator

No description provided.

@Thomasdezeeuw Thomasdezeeuw mentioned this pull request Jul 11, 2019
@Thomasdezeeuw
Copy link
Collaborator Author

Creates the following output (for the functional tests):


     Running target/debug/deps/functional-740432e6e33d57cd

running 1 test
test tests::poll::test_poll_closes_fd ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

The we would have three categories; unit, functional and end to end, and of course the doc tests. Seems pretty good to me.

@Thomasdezeeuw
Copy link
Collaborator Author

Cargo fmt fails with Error writing files: io error: Failed to find module poll in "functional" None. Rustfmt rustfmt doesn't understand the path attribute: rust-lang/rustfmt#1208.

@asomers
Copy link
Collaborator

asomers commented Jul 11, 2019

Cool! What happens if you remove the path attribute?

@Thomasdezeeuw
Copy link
Collaborator Author

Then we can't import poll, as it will look for tests/poll.rs or tests/poll/mod.rs.

@asomers
Copy link
Collaborator

asomers commented Jul 11, 2019

Well, what would happen if you eliminated both #[path] and the mod tests?

@Thomasdezeeuw
Copy link
Collaborator Author

I'm stupid. I started with just having mod poll (not in a module) in the file and that didn't work. But just adding mod functional { mod poll } works, no need for path.

@Thomasdezeeuw
Copy link
Collaborator Author

@carllerche you also ok with this layout? If so I'll update #1030 with this structure.

@Thomasdezeeuw
Copy link
Collaborator Author

Ok, rustfmt failed again but this this it's just a bug I think.

@Thomasdezeeuw
Copy link
Collaborator Author

Thomasdezeeuw commented Jul 11, 2019

I think its rust-lang/rustfmt#3572.

@carllerche
Copy link
Member

@Thomasdezeeuw I'm fine w/ the structure, but how do you plan to deal w/ the rustfmt bug?

@Thomasdezeeuw
Copy link
Collaborator Author

I don't yet, I've just asked in rust-lang/rustfmt#3572 for a work around. Maybe we should just merge #1019 as is and move this structure if/when we find a work around or a fix.

@carllerche
Copy link
Member

@Thomasdezeeuw Yeah, lets just merge #1019 and wait for a fix. Thanks for trying out this option 👍

@Thomasdezeeuw
Copy link
Collaborator Author

I've force pushed some changes, these use a (too complicated) macro to set the path correctly for each file as a workaround for the rustfmt bugs. A downside is that I'm pretty sure that rustfmt doesn't reach and formats the files anymore, but at least it won't return an error.

@Thomasdezeeuw
Copy link
Collaborator Author

What you think about the macro approach? It isn't great, but I don't really see another way to get it to work currently.

@Thomasdezeeuw Thomasdezeeuw changed the title Test refactor2 Test refactor continued Jul 16, 2019
@carllerche
Copy link
Member

Up to you, but IMO it isn't worth going against the grain and I would just stick with #1019.

@Thomasdezeeuw
Copy link
Collaborator Author

Rustfmt fails with yet anther issue, but I wanted to get some feedback first. @carllerche, @asomers what do you think of the new output.

@carllerche
Copy link
Member

My vote is still to follow cargo idioms, but I will go with whatever :)

@asomers
Copy link
Collaborator

asomers commented Jul 20, 2019

I like it. It's just what I had in mind. Too bad about that annoying rustfmt bug, though.

@Thomasdezeeuw
Copy link
Collaborator Author

The last commit works around the last rustfmt bug, however Azure doesn't seem to want to run. Any way to start it manually?

@Thomasdezeeuw
Copy link
Collaborator Author

I'm closing this for now, to many rebase conflicts.

@Thomasdezeeuw Thomasdezeeuw deleted the test-refactor2 branch December 4, 2019 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants