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

Move compile-fail tests with NOTE/HELP annotations to UI #46641

Merged
merged 6 commits into from
Dec 15, 2017

Conversation

petrochenkov
Copy link
Contributor

Remove NOTE/HELP annotations from UI tests

cc #44844 @oli-obk @est31
r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

This looks good. We should however document the expected policy, I think, to the compiler test documentation.

I guess the idea is:

  • Use ui tests and the stderr files to document notes etc.

I will note one thing (which should not hold up this PR). I've been very much enjoying the fact that UI tests check for ERRORs. I like being able to add minimal annotations to the test that give the kind of "overall parameters" for what the test is testing (that errors occur here, here, and here) and then having the stderr files capture the full behavior.

But there are times when I want to document a subset of the NOTEs that are present. i.e., so that I can outline "other variations in the stderr output may be acceptable, but if this NOTE disappears, that's a big problem". Note that I say subset -- often there are many notes, but it's just one piece I want to guarantee is present.

Before, we required that if you document any note, you must document all notes. The result is that it discourages me from annotating these notes that I want to be present.

(On the other hand, if you can't declare that the set of notes is exhaustive, you can't document that a note should not appear. I don't find this to be something I want to document very often, but it's certainly possible. But we could easily have a "NO_NOTE" annotation or something then.)

What I do for now is to leave comments, but I fear that future people will not read them.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2017

What I do for now is to leave comments, but I fear that future people will not read them.

Maybe it would be better suited for an issue with the appropriate tags? Do we have testsuite tags?

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 11, 2017

📌 Commit 8f58682 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@oli-obk

Maybe it would be better suited for an issue with the appropriate tags? Do we have testsuite tags?

I don't follow. Maye what would be better suited?

@kennytm kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 11, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2017

Sorry, I kind of skipped ahead.

Adding a way to make these comments annotations that can't be ignored is certainly something that makes sense. It'll prevent accidental changes down the road. What I meant was make this comment an issue so it's not forgotten.

@bors
Copy link
Contributor

bors commented Dec 11, 2017

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

@nikomatsakis
Copy link
Contributor

@oli-obk

Adding a way to make these comments annotations that can't be ignored is certainly something that makes sense. It'll prevent accidental changes down the road. What I meant was make this comment an issue so it's not forgotten.

yep =) #46667

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 12, 2017
@petrochenkov
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 12, 2017

📌 Commit 2820616 has been approved by nikomatsakis

@petrochenkov petrochenkov 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2017
@bors
Copy link
Contributor

bors commented Dec 13, 2017

⌛ Testing commit 28206165f20f9aab2eb1bdc9e27e93e64b266369 with merge 634341d84dbfa933c5ef2d6b41b9c9e554294d25...

@bors
Copy link
Contributor

bors commented Dec 13, 2017

💔 Test failed - status-travis

@petrochenkov
Copy link
Contributor Author

Curl failure, looks spurious.
@bors retry

@bors
Copy link
Contributor

bors commented Dec 13, 2017

⌛ Testing commit 28206165f20f9aab2eb1bdc9e27e93e64b266369 with merge dc02dfe1058a0af48ebd811a8f744e6ef26500c9...

@bors
Copy link
Contributor

bors commented Dec 13, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Dec 13, 2017

@bors retry — http://crosstool-ng.org was down.

@bors
Copy link
Contributor

bors commented Dec 14, 2017

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

@petrochenkov
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 14, 2017

📌 Commit 66bd53a has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 15, 2017

⌛ Testing commit 66bd53a with merge 2f35141...

bors added a commit that referenced this pull request Dec 15, 2017
Move compile-fail tests with NOTE/HELP annotations to UI

Remove NOTE/HELP annotations from UI tests

cc #44844 @oli-obk @est31
r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2f35141 to master...

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.

5 participants