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.

Sign up for GitHub

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

Jump to bottom

compiletest: mir-opt tests don't warn about stale output relative to source timestamp #39690

Closed
pnkfelix opened this issue Feb 9, 2017 · 1 comment
Closed

compiletest: mir-opt tests don't warn about stale output relative to source timestamp #39690

pnkfelix opened this issue Feb 9, 2017 · 1 comment
Assignees
@pnkfelix
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Feb 9, 2017

Right now, if you work on a mir-opt test and iteratively change it without cleaning in between runs, your build directory can end up with content like this:

 % ls -o *TypeckMir.before.mir )
-rw-r--r-- 1 pnkfelix 5759 Feb  9 15:42 rustc.node13.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 2773 Feb  9 14:42 rustc.node16.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 4373 Feb  9 15:42 rustc.node20.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 2817 Feb  9 14:54 rustc.node21.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 2816 Feb  9 14:56 rustc.node22.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix  458 Feb  9 14:41 rustc.node29.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix  458 Feb  9 14:54 rustc.node36.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix  458 Feb  9 14:56 rustc.node37.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 2800 Feb  9 15:14 rustc.node40.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 2800 Feb  9 15:42 rustc.node43.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 3228 Feb  9 14:56 rustc.node4.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix  844 Feb  9 15:14 rustc.node55.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix  844 Feb  9 15:42 rustc.node58.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix 2772 Feb  9 14:41 rustc.node7.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix  458 Feb  9 15:14 rustc.node86.TypeckMir.before.mir
-rw-r--r-- 1 pnkfelix  458 Feb  9 15:42 rustc.node89.TypeckMir.before.mir

Note the wide variety of timestamps. As the node id's changed in response to source file modifications, some of the output stops getting overwritten and just stays in place.

This, combined with the mir-opt architecture where one embeds the name of the specific file that one wants to compare against, has a bad failure case during iterative development: if you are not paying attention and leave the header as e.g. START rustc.node4.TypeckMir.before.mir, you'll keep comparing your expected output against the same (stale) file, so you'll either get false negatives (the test passes because the embedded expected output is just as stale as the file you end up comparing against), or as you revise your concrete test input, you'll get false positives (the test fails because the now correct embedded expected output is still being compared against a stale file).

The user error here is of course that the START rustc.node4.TypeckMir.before.mir needs to be kept in sync with the actual node number to which the mir dump sends its output.

But it would be much better if compiletest did a sanity check on the time stamps: if the dumped mir output is older than the original source file, then chances are something has gone wrong, and compile test could either error in response, or at least issue a warning to the user.
@pnkfelix pnkfelix self-assigned this Feb 9, 2017
@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-testsuite Area: The testsuite used to check the correctness of rustc labels Feb 9, 2017
@pnkfelix
Copy link
Member Author

(I have a patch for this; will post shortly)

@pnkfelix pnkfelix mentioned this issue Feb 10, 2017
Merged
pnkfelix added a commit to pnkfelix/rust that referenced this issue Feb 20, 2017
@pnkfelix
Fix rust-lang#39690: make mir-opt mode for compiletest check timestamps.
6a78282 
This version removes prior use of `time` crate, to satisify vendoring requirements.

remove extraneous whitespace change
bors added a commit that referenced this issue Feb 20, 2017
@bors
Auto merge of #39717 - pnkfelix:check-timestamps-in-compiletest-mirop…
a17e5e2 
…t, r=alexcrichton

When compiletest'ing src/test/mir-opt, check timestamps.

The tests in src/test/mir-opt embed references to generated files. The names of the generated files embed node id's, which will change depending on the content of the original source.

To guard against comparisons against stale output, check the timestamps of the supposed output against the timestamp of the original source (i.e. any output should be at least as new as the source that was recompiled).

Fix #39690.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pnkfelix pnkfelix

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@pnkfelix