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 quote tests #24718

Merged
merged 4 commits into from
Apr 26, 2015
Merged

Fix quote tests #24718

merged 4 commits into from
Apr 26, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Apr 23, 2015

Sniped from @rprichard's work in #24537. r? @alexcrichton


#![feature(quote)]
#![feature(quote, rustc_private)]

extern crate syntax;
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @rprichard that these tests which link to syntax should be in compile-fail-fulldeps to ensure that the dependencies work out correctly during builds.

@alexcrichton
Copy link
Member

Thanks!


fn check_pp<T>(expr: T, f: |pprust::ps, T|, expect: str) {
panic!();
quote_expr!(&cx, let x isize = 20;).and_then(|expr| { //~ ERROR expected end-of-string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton do you have any thoughts on how this can be made to fail in the expected way? it currently compiles without issue and then crashes at runtime

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I know very little about the quasiquoter so I'm not sure :(

@tamird tamird force-pushed the fix-quote-tests branch 3 times, most recently from 8ac3341 to a8b2382 Compare April 23, 2015 16:34
@tamird
Copy link
Contributor Author

tamird commented Apr 23, 2015

OK, I think this should be good to merge after #24537 goes in.

@alexcrichton
Copy link
Member

@bors: r+ a8b2382

(hopefully bors will order these right)

@tamird
Copy link
Contributor Author

tamird commented Apr 23, 2015

Apparently it won't, because @rprichard's branch is marked rollup.
On Apr 23, 2015 10:05 AM, "Alex Crichton" notifications@github.com wrote:

@bors https://github.com/bors: r+ a8b2382
a8b2382

(hopefully bors will order these right)


Reply to this email directly or view it on GitHub
#24718 (comment).

@alexcrichton
Copy link
Member

@bors: rollup

muahahaha

Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 24, 2015
bors added a commit that referenced this pull request Apr 24, 2015
bors added a commit that referenced this pull request Apr 24, 2015
@Manishearth
Copy link
Member

This fails, but 874e093 might fix it. Please check locally.

@tamird
Copy link
Contributor Author

tamird commented Apr 24, 2015

not quite - run-fail/qquote.rs was missing backticks around 'abcd' in the expected error

@alexcrichton
Copy link
Member

@bors: r= 1cc7b51

@alexcrichton
Copy link
Member

@bors: r+ 1cc7b51

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Apr 24, 2015
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Apr 24, 2015
@steveklabnik
Copy link
Member

@steveklabnik
Copy link
Member

@bors: r-

@tamird
Copy link
Contributor Author

tamird commented Apr 25, 2015

Ugh, the expected error message had a superfluous colon. Should be good now.

@tamird
Copy link
Contributor Author

tamird commented Apr 25, 2015

This passes locally now!

@steveklabnik
Copy link
Member

@bors: r=alexcrichton

@bors
Copy link
Contributor

bors commented Apr 25, 2015

📌 Commit beb373b has been approved by alexcrichton

@rprichard
Copy link
Contributor

I feel we should leave the test disabled until we've figured out a way to enable it that doesn't have missing dependencies. Presumably this change will break parallel make check.

@tamird
Copy link
Contributor Author

tamird commented Apr 25, 2015

@rprichard I'm trying to induce this failure: make -j20 check-stage1-rfail. In the meantime, would you mind putting up a commit I could look at that adds rfail-full?

@rprichard
Copy link
Contributor

Sure, I'll take a stab at it.

@bors
Copy link
Contributor

bors commented Apr 25, 2015

⌛ Testing commit beb373b with merge da62384...

bors added a commit that referenced this pull request Apr 25, 2015
@rprichard
Copy link
Contributor

Here's a commit adding rfail-full. rprichard@55e8d5e. I haven't moved the quote test over, though I probably should. Then, I can get rid of src/tests/run-fail-fulldeps/.gitkeep and not worry about what an empty test directory does. I'm doing some testing with it.

I was able to reproduce a failure with make -j10 check-stage1

failures:

---- [run-fail] run-fail/qquote.rs stdout ----

error: compilation failed!
status: exit code: 101
command: x86_64-unknown-linux-gnu/stage1/bin/rustc /home/rprichard/work/rust-staging2/src/test/run-fail/qquote.rs -L x86_64-unknown-linux-gnu/test/run-fail/ --target=x86_64-unknown-linux-gnu -L x86_64-unknown-linux-gnu/test/run-fail/qquote.stage1-x86_64-unknown-linux-gnu.run-fail.libaux -C prefer-dynamic -o x86_64-unknown-linux-gnu/test/run-fail/qquote.stage1-x86_64-unknown-linux-gnu --cfg rtopt --cfg ndebug -C rpath -O -L x86_64-unknown-linux-gnu/rt
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
/home/rprichard/work/rust-staging2/src/test/run-fail/qquote.rs:17:1: 17:21 error: can't find crate for `syntax`
/home/rprichard/work/rust-staging2/src/test/run-fail/qquote.rs:17 extern crate syntax;
                                                                  ^~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

------------------------------------------

thread '[run-fail] run-fail/qquote.rs' panicked at 'explicit panic', /home/rprichard/work/rust-staging2/src/compiletest/runtest.rs:1512



failures:
    [run-fail] run-fail/qquote.rs

test result: FAILED. 97 passed; 1 failed; 5 ignored; 0 measured

thread '<main>' panicked at 'Some tests failed', /home/rprichard/work/rust-staging2/src/compiletest/compiletest.rs:256
make: *** [tmp/check-stage1-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-rfail.ok] Error 101
make: *** Waiting for unfinished jobs....

@tamird
Copy link
Contributor Author

tamird commented Apr 26, 2015

Nice, thanks! Since this PR is already building, do you think we could
address this in a follow-up PR? You could then include the reordering
mentioned in your commit in a separate commit.
On Apr 25, 2015 5:26 PM, "Ryan Prichard" notifications@github.com wrote:

Here's a commit adding rfail-full. rprichard/rust@55e8d5e
rprichard@55e8d5e.
I haven't moved the quote test over, though I probably should. Then, I can
get rid of src/tests/run-fail-fulldeps/.gitkeep and not worry about what
an empty test directory does. I'm doing some testing with it.

I was able to reproduce a failure with make -j10 check-stage1

failures:

---- [run-fail] run-fail/qquote.rs stdout ----

error: compilation failed!
status: exit code: 101
command: x86_64-unknown-linux-gnu/stage1/bin/rustc /home/rprichard/work/rust-staging2/src/test/run-fail/qquote.rs -L x86_64-unknown-linux-gnu/test/run-fail/ --target=x86_64-unknown-linux-gnu -L x86_64-unknown-linux-gnu/test/run-fail/qquote.stage1-x86_64-unknown-linux-gnu.run-fail.libaux -C prefer-dynamic -o x86_64-unknown-linux-gnu/test/run-fail/qquote.stage1-x86_64-unknown-linux-gnu --cfg rtopt --cfg ndebug -C rpath -O -L x86_64-unknown-linux-gnu/rt

stdout:


stderr:

/home/rprichard/work/rust-staging2/src/test/run-fail/qquote.rs:17:1: 17:21 error: can't find crate for syntax
/home/rprichard/work/rust-staging2/src/test/run-fail/qquote.rs:17 extern crate syntax;
^~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error


thread '[run-fail] run-fail/qquote.rs' panicked at 'explicit panic', /home/rprichard/work/rust-staging2/src/compiletest/runtest.rs:1512

failures:
[run-fail] run-fail/qquote.rs

test result: FAILED. 97 passed; 1 failed; 5 ignored; 0 measured

thread '

' panicked at 'Some tests failed', /home/rprichard/work/rust-staging2/src/compiletest/compiletest.rs:256
make: *** [tmp/check-stage1-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-rfail.ok] Error 101
make: *** Waiting for unfinished jobs....


Reply to this email directly or view it on GitHub
#24718 (comment).

@rprichard
Copy link
Contributor

I could go either way. We might want to avoid having a span of time where make check-stage1 is unreliable on master, so my inclination would be to cherry-pick my commit into this PR. I can submit a later PR reordering the tests, if it seems worth it. I'm not sure, but I think it's possible that check-stage1 could fail in practice, even without parallelism, because I think make rustc-stage1 doesn't build the rustc lib crates, and I think the rfail test might run before any fulldeps tests.

I verified that make -j10 check-stage1-rfail-full passes with that commit, BTW. As I guessed, it succeeds and runs zero tests. If I remove the .gitkeep file (and therefore the src/tests/run-fail-fulldeps directory), the tests fail with:

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os(2) }', /home/rprichard/work/rust-staging3/src/libcore/result.rs:729
make: *** [tmp/check-stage1-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-rfail-full.ok] Error 101

@rprichard
Copy link
Contributor

In any case, I removed my .gitkeep file and moved qquote. It's on this branch: master...rprichard:fix-quote-tests. I'm running some tests on it to verify that it works.

@bors bors merged commit beb373b into rust-lang:master Apr 26, 2015
@tamird tamird deleted the fix-quote-tests branch April 26, 2015 02:21
@tamird
Copy link
Contributor Author

tamird commented Apr 26, 2015

@rprichard mind opening that PR or do you want me to?

@rprichard
Copy link
Contributor

I'll open the PR. I verified that make -j6 rustc-stage1 && make check-stage1 fail with a missing syntax crate with this PR, so I'll get a PR ready within a few hours. My fixed version also failed, on the run-fail-fulldeps/qquote.rs test, apparently because it's trying to access some path that doesn't exist.

@rprichard
Copy link
Contributor

I needed to add a line to configure. I'll see if that fixes things.

bors added a commit that referenced this pull request Apr 28, 2015
This commit gets `make check-stage1` working again after #24718.

cc @tamird

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.

6 participants