-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Number of filtered out tests in tests summary #41910
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Changes look good to me. Could you squash your changes into one commit? Thanks! |
@Mark-Simulacrum I'm not sure how. "Merge pull request" button is not availible for me, I don't know other ways so squash commits in pull request other then this button's variant. UPD: Figured it out with |
Sorry about that. I should have provided instructions on how to squash. Anyway, changes look good. @bors r+ |
📌 Commit e6cde9f has been approved by |
@Mark-Simulacrum no problem at all, I shouldn't have asking before googling it in the first place :) |
Number of filtered out tests in tests summary Closes rust-lang#31905
Number of filtered out tests in tests summary Closes rust-lang#31905
Test failed due to Cargo. https://github.com/rust-lang/cargo/blob/d8fa3eb4/tests/bench.rs#L51 See #41930 (comment). |
Would you mind sending a PR to cargo to add the expected output there? |
err, @mersinvald that is. |
@bors r- (since this breaks Cargo's tests) |
@Mark-Simulacrum sure, I will. I should just reference this issue in the PR to cargo, right? |
Yeah, that should be fine. I'm not entirely sure how the dance with the submodule will work, though, seeing as with your PR Cargo's tests will presumably fail. But once the Cargo PR is up, ping me and @alexcrichton on it, and we should be able to work it out. |
@Mark-Simulacrum can you assist me through cargo testing process? How can I build cargo with my rustc version and run tests? I have new rustc in my PATH:
When I do
But cargo seems to use new rustc (I see N filtered out messages for other tests) How can I fix it? |
Oh, I'm sorry I somehow missed your comment! I'm not actually entirely sure how to best do that. @alexcrichton Could you provide some guidance here? I don't know how to sync up Cargo with rustc so that the tests pass there before we merge this and vice-versa. |
@mersinvald are you using rustup perhaps? If that's the case then the |
@mersinvald So I've talked to @alexcrichton on IRC, and for the Cargo PR, you should just change the tests to be less strict about libtest's output so they don't break on the current and the future output (with this PR). |
Any opinions on using |
…chton Making tests less strict so they won't break on output changes Required for rust-lang/rust#41910 cc @Mark-Simulacrum cc @alexcrichton
…chton Making tests less strict so they won't break on output changes Required for rust-lang/rust#41910 cc @Mark-Simulacrum cc @alexcrichton
@Mark-Simulacrum hello! Can you give me update on merging process? Pull Request to cargo is merged now. Are there some difficulties in merging this one? |
I think you'll need to put up a PR that updates the cargo submodule in this repository. Once that's merged, I think we can try merging this. |
@Mark-Simulacrum am I right that I just need to do |
I think so. I've never personally done it, but I believe the thing to Google for if that doesn't work is something along the lines of how to update a submodule. |
…r=Mark-Simulacrum Cargo submodule update Required for rust-lang#41910 r? @Mark-Simulacrum
…lacrum Cargo submodule update Required for #41910 r? @Mark-Simulacrum
@bors r+ In theory this should work now... |
📌 Commit e6cde9f has been approved by |
💔 Test failed - status-appveyor |
Seems to have timed out? Or ... something? I don't see any failures @bors retry |
@Mark-Simulacrum ah yeah our time limit is 3 hours on AppVeyor, and the failing build ran for 3 hours, so likely a timeout. |
⌛ Testing commit e6cde9f with merge 55bb4c9... |
💔 Test failed - status-appveyor |
@bors: retry This seems to keep getting unlucky :( |
💔 Test failed - status-appveyor |
Log
|
Number of filtered out tests in tests summary Closes rust-lang#31905
Closes #31905