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

ui_test: more robust comment parsing #2170

Closed
RalfJung opened this issue May 31, 2022 · 7 comments · Fixed by #2332
Closed

ui_test: more robust comment parsing #2170

RalfJung opened this issue May 31, 2022 · 7 comments · Fixed by #2332
Labels
A-tests Area: affects our test suite or CI C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps

Comments

@RalfJung
Copy link
Member

RalfJung commented May 31, 2022

Currently, if we fail to parse a //~ line or an annotation like // ignore-target, we just go on. That's a pretty bad default for infrastructure that is meant to test our code! For both of these I think we should hard error if we cannot parse the line.

For //~ that is fairly easy; if any comment starting //~ eludes us we can just error.

For the other annotations it is a lot harder since that might just be a regular comment. I think we need to change their syntax so that we can tell that this line is meant to be special. //@ might work? We should ideally sync this with compiletest, in case they have similar plans.

@oli-obk
Copy link
Contributor

oli-obk commented May 31, 2022

Compiletest wants to become more robust/explicit here, too: rust-lang/compiler-team#513

@RalfJung
Copy link
Member Author

That sounds somewhat orthogonal -- that is about adding extra annotations that compiletest leaves uninterpreted, which is different from being able to tell when the user has made a typo in their annotation.

@oli-obk
Copy link
Contributor

oli-obk commented May 31, 2022

Hmm... seems the same to me. If we don't have a sigil like @ making sure that we must successfully parse, anything like // ingore-x86 will just silently be ignored (pun intended)

@RalfJung
Copy link
Member Author

RalfJung commented May 31, 2022

In my understanding, that issue is about // annotations. As in, 'annotations' is a keyword, not a stand-in for "any kind of annotation".

@RalfJung RalfJung added the C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement label Jun 5, 2022
@RalfJung
Copy link
Member Author

RalfJung commented Jun 9, 2022

Ah, this is the compiletest proposal you probably meant: rust-lang/compiler-team#512. That's probably where I got //@ from.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 8, 2022

Reopening since the transition is still ongoing. :)

@RalfJung RalfJung added the C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps label Jul 8, 2022
@RalfJung
Copy link
Member Author

RalfJung commented Jul 8, 2022

Oh wait, you have ported all tests. I didn't realize that. Wow :)

@RalfJung RalfJung closed this as completed Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tests Area: affects our test suite or CI C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants