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

Add bless x.py subcommand for easy ui test replacement #50806

Merged
merged 5 commits into from
May 18, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 16, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented May 16, 2018

cc @ehuss

@kennytm
Copy link
Member

kennytm commented May 16, 2018

With the implementation in this PR, I think it should be a --bless flag to test instead of an entire new subcommand.

$ ./x.py test src/test/ui --bless

In #49815's suggestion the subcommand works like update-reference.sh. These scripts won't run tests themselves, they only copy the generated stdout/stderr files, meaning the workflow would be like

$ ./x.py test src/test/ui

(... fix errors ...)

$ ./x.py test src/test/ui

(... fix errors ...)

$ ./x.py test src/test/ui

(done)

$ ./x.py bless src/test/ui # <- will not run tests again

@petrochenkov
Copy link
Contributor

petrochenkov commented May 16, 2018

In my experience the workflow is

// 1. Initial run
x.py test ui
// 2. Update stderr/stdout
update-all-reference.sh
// 3. Was it enough or some `//~ ERROR` annotations need update as well?
x.py test ui
// 4. Optionally: Fix `//~ ERROR` annotations
(... fix errors ...)
// 5. Was it enough or stderr/stdout need updating again?
x.py test ui
// 6. Optionally: Update stderr/stdout
update-all-reference.sh
// 7. Make sure everything passes now, maybe go back to `4.` if not
x.py test ui

so I'd very much prefer to do the "test-update-test" operation in one command to keeping the "update" logic separate.
Then the workflow would turn into

// 1. Initial "test-update-test" run
x.py bless ui
// 2. Optionally: Fix `//~ ERROR` annotations
(... fix errors ...)
// 3. Second "test-update-test" run to make sure everything passes now, maybe go back to `2.` if not
x.py bless ui

I'm not sure about test --bless vs bless, the first one is more correct because it's "test with a tweak", but the second one is more convenient.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 16, 2018

Yes that is also my workflow. I changed it to a flag, as that makes the code simpler and I already pass such a mess of flags to x.py it doesn't make a difference to me.

RUST_BACKTRACE=1 ./x.py test --stage 1 src/test/ui --test-args const_signed_pat --bless

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:49:31]    Compiling ansi_term v0.11.0
[00:49:31]    Compiling difference v2.0.0
[00:49:32]    Compiling pretty_assertions v0.5.1
[00:49:32]    Compiling bootstrap v0.0.0 (file:///checkout/src/bootstrap)
[00:49:37] error[E0063]: missing field `bless` in initializer of `flags::Subcommand`
[00:49:37]     --> bootstrap/builder.rs:1450:22
[00:49:37]      |
[00:49:37] 1450 |         config.cmd = Subcommand::Test {
[00:49:37]      |                      ^^^^^^^^^^^^^^^^ missing `bless`
[00:49:37] error: aborting due to previous error
[00:49:37] 
[00:49:37] For more information about this error, try `rustc --explain E0063`.
[00:49:37] For more information about this error, try `rustc --explain E0063`.
[00:49:37] error: Could not compile `bootstrap`.
[00:49:37] To learn more, run the command again with --verbose.
[00:49:37] 
[00:49:37] 
[00:49:37] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--"
[00:49:37] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--"
[00:49:37] expected success, got: exit code: 101
[00:49:37] 
[00:49:37] 
[00:49:37] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:49:37] Build completed unsuccessfully in 0:00:21
[00:49:37] make: *** [check] Error 1
[00:49:37] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:045cdfe4
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! Sorry I didn't get to this sooner! (BTW, love the branch name.)

Maybe the error message that says "To update references, run this command..." should be updated to the new command?

Also, beware that there are a bunch of path changes coming in #50400. There shouldn't be any fundamental problems, but some of the function names will change.

)),
let mut files = vec![output_file];
if self.config.bless {
files.push(self.expected_output_path(kind));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work for nll files. For an example, copy E0508.rs to src/test/ui and try to bless it. You should end up with these three files:

E0508.ast.nll.stderr
E0508.ast.stderr
E0508.mir.stderr

I think the issue is that expected_output_path doesn't include mode if the file doesn't already exist. I think the solution is to call the global expected_output_path (not the self version).

@nikomatsakis
Copy link
Contributor

@ehuss -- you're the one most familiar with the code at this point, would you be up for tackling the review here? (I can delegate to you from bors)

@ehuss
Copy link
Contributor

ehuss commented May 17, 2018

@nikomatsakis I don't mind, but I don't have any more comments beyond what I posted.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 17, 2018

@bors delegate=ehuss

@bors
Copy link
Contributor

bors commented May 17, 2018

✌️ @ehuss can now approve this pull request

@killercup
Copy link
Member

@oli-obk want to add replacement of .fixed files, too? (not necessarily in this pr though)

@bors
Copy link
Contributor

bors commented May 17, 2018

☔ The latest upstream changes (presumably #50400) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 17, 2018

@killercup that should automagically work with this PR ;) There's no code specific to stderr files in it.

@nikomatsakis
Copy link
Contributor

@oli-obk are you ready for a re-review from @ehuss then?

@oli-obk
Copy link
Contributor Author

oli-obk commented May 17, 2018

Yea, I was just waiting for travis :) Last time I broke some tests that ran very late in the pipeline.

@ehuss
Copy link
Contributor

ehuss commented May 17, 2018

@bors: r+

Nice!

@bors
Copy link
Contributor

bors commented May 17, 2018

📌 Commit 0ac2fd1 has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2018
@Mark-Simulacrum
Copy link
Member

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 17, 2018
Add `bless` x.py subcommand for easy ui test replacement

fixes rust-lang#49815

r? @nikomatsakis
bors added a commit that referenced this pull request May 18, 2018
Rollup of 10 pull requests

Successful merges:

 - #50387 (Remove leftover tab in libtest outputs)
 - #50553 (Add Option::xor method)
 - #50610 (Improve format string errors)
 - #50649 (Tweak `nearest_common_ancestor()`.)
 - #50790 (Fix grammar documentation wrt Unicode identifiers)
 - #50791 (Fix null exclusions in grammar docs)
 - #50806 (Add `bless` x.py subcommand for easy ui test replacement)
 - #50818 (Speed up `opt_normalize_projection_type`)
 - #50837 (Revert #49767)
 - #50839 (Make sure people know the book is free oline)

Failed merges:
@scottmcm
Copy link
Member

Thank you especially from those of us who run on windows without a shell to run the .sh 🎉

@bors bors merged commit 0ac2fd1 into rust-lang:master May 18, 2018
@oli-obk oli-obk deleted the gesundheit branch May 18, 2018 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move the update-reference.sh script into x.py
10 participants