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

fix JSON error test expectation for lint changes in rust-lang/rust#38103 #3513

Conversation

zackmdavis
Copy link
Member

Appveyor CI pointed out that rust-lang/rust#38103 (which has been r+'d) breaks Cargo's compiler_json_error_format. I don't know if we have a procedure for coordinating these kinds of changes across repos in a way that CI understands, but ideally, this PR would land here in parallel with rust-lang/rust#38103 in the rust repo.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

@alexcrichton
Copy link
Member

Thanks for the PR! So far the coordination looks like you've figured out. We basically just update Cargo here, then update the submodule, then land the original rust-lang/rust PR.

Unfortunately, though, this PR will have to pass the current test suite of Cargo, which is being run against a compiler without the changes here. That means that as-is we can't land the PR here I believe.

Perhaps we can just relax the test case here? We don't really need to exhaustively check for precisely what the compiler is printing, but from Cargo's perspective we're just testing that something was printed and it was printed to the right place.

@bors
Copy link
Contributor

bors commented Jan 10, 2017

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

Checking the exact JSON output is very brittle, making it impossible to
make changes to rustc lint output without first landing a change to this
test. If we just check that lines of valid JSON are emitted, that should
suffice for Cargo's purposes, without holding up exciting iterations on
linting in the compiler itself, such as rust-lang/rust#38103.
@zackmdavis zackmdavis force-pushed the fix_test_conditional_on_rustc_pr_38103 branch from 73c43be to 740088e Compare January 11, 2017 05:22
@zackmdavis
Copy link
Member Author

@alexcrichton (force-pushed)

Perhaps we can just relax the test case here? We don't really need to exhaustively check for precisely what the compiler is printing, but from Cargo's perspective we're just testing that something was printed and it was printed to the right place.

... and at least check that the stdout lines are valid JSON? (740088e)

@alexcrichton
Copy link
Member

@zackmdavis oh hm I don't think we want to remove all of these assertions, some of them are internal to Cargo itself and we want to continue asserting they're printed correctly. Perhaps just the rustc-specific bits could get ignored?

@zackmdavis
Copy link
Member Author

Maybe ... what makes this really awkward is that the existing execs().with_json() matcher doesn't have any way of ignoring part of a JSON structure (the [..] wildcard functionality is only within strings). Rather than trying to implement JSON-substructure wildcards, I started just using rustc_serialize to pull apart the JSON and make the necessary assertions directly in the compiler_json_error_format test itself (work not committed yet because there are some "borrowed value does not live long enough" errors that I'm too heartbroken and frustrated to look at right now), which should work, but has the drawback of being utterly hideous—

    let stdout = p.cargo_process("build").arg("-v")
        .arg("--message-format").arg("json").exec_with_output()
        .expect("process should succeed").stdout;
    let stdout_lines = stdout.split(|&c| c == ('\n' as u8)).filter(|l| l.len() != 0);

    let bar_message_line = str::from_utf8(stdout_lines.next().unwrap())
        .expect("output should be valid UTF-8");
    let bar_message_object = Json::from_str(bar_message_line)
        .expect("line should be valid JSON").as_object().unwrap();
    assert_eq!("compiler-message", bar_message_object.get("reason").unwrap().as_string().unwrap());
    assert_eq!("bar",
               bar_message_object.get("target").unwrap().as_object().unwrap()
                   .get("name").unwrap().as_string().unwrap());

    let bar_artifact_line = str::from_utf8(stdout_lines.next().unwrap())
        .expect("output should be valid UTF-8");
    let bar_artifact_object = Json::from_str(bar_message_line)
        .expect("line should be valid JSON").as_object().unwrap();
    let expected_bar_artifact_object = Json::from_str(r##"                                                                                                                                      
     {                                                                                                                                                                                          
         "reason":"compiler-artifact",                                                                                                                                                          
         "profile": {                                                                                                                                                                           
             "debug_assertions": true,                                                                                                                                                          
             "debuginfo": true,                                                                                                                                                                 
             "opt_level": "0",                                                                                                                                                                  
             "test": false                                                                                                                                                                      
         },                                                                                                                                                                                     
         "features": [],                                                                                                                                                                        
         "package_id":"bar 0.5.0 ([..])",                                                                                                                                                       
         "target":{"kind":["lib"],"name":"bar","src_path":"[..]lib.rs"},                                                                                                                        
         "filenames":["[..].rlib"]                                                                                                                                                              
     }                                                                                                                                                                                          
"##).unwrap().as_object().unwrap();
    assert_eq!(expected_bar_artifact_object, bar_artifact_object);
// &c.

(I'm kind of upset about my beautiful lint-group-message change being blocked on this much yak-shaving to loosen a test expectation in a completely different repo, but such is the nature of our work, "Patch or STFU" being the whole of the moral law.)

@alexcrichton
Copy link
Member

Sorry this is becoming a bit of a yak shave!

We've already got a function comparing two JSON objects, perhaps we could take a hack and say that NaN is equivalent to "any json object here", so it's essentially the equivalent of [..]

@zackmdavis
Copy link
Member Author

out of open-source time this week; will try to take a look at JSON wildcarding (with NaN or some other way) starting Monday night at earliest

@bors
Copy link
Contributor

bors commented Jan 13, 2017

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

@zackmdavis
Copy link
Member Author

superseded by #3628

@zackmdavis zackmdavis closed this Feb 1, 2017
bors added a commit that referenced this pull request Feb 3, 2017
…ain, r=alexcrichton

make build tests not depend on minutiae of rustc output

This little patch arises from the maelstrom of my purity born of pain.

It's the pain of seeing rust-lang/rust#38103 in its perfect crystalline beauty waste away on page four of https://github.com/rust-lang/rust/pulls, waiting, ready, itching to land, dying with anticipation to bring the light of clearer lint group error messages to Rust users of all creeds and nations, only for its promise to be cruelly blocked by the fateful, hateful hand of circular dependency. For it is written in src/tools/cargotest/main.rs that the Cargo tests must pass before the PR can receive Appveyor's blessing, but the Cargo tests could not pass (because they depend on fine details of the output that the PR is meant to change), and the Cargo tests could not be changed (because updating the test expectation to match the proposed new compiler output, would fail with the current compiler).

The Gordian knot is cut in the bowels of cargotest's very notion of comparison (of JSON objects) itself, by means of introducing a magic string literal `"{...}"`, which can server as a wildcard for any JSON sub-object.

And so it will be for the children, and the children's children, and unto the 1.17.0 and 1.18.0 releases, that Cargo's build test expectations will faithfully expect the exact JSON output by Cargo itself, but the string literal `"{...}"` shall be a token upon the JSON output by rustc, and when I see `"{...}"`, I will pass over you, and the failure shall not be upon you.

And this day shall be unto you for a memorial.

supersedes #3513

r? @alexcrichton
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.

4 participants