-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Tidying up tests/ui/issues 33 tests [4/N] #149767
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
base: main
Are you sure you want to change the base?
Conversation
|
This PR modifies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review and I wanted to ask: how many tests per PR would be most convenient for you to review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there a directory for tests that test a CLI options passed to compiler like in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an invalid-compile-flags directory, but no directory like run-pass compile flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do tests/ui/compile-flags and split it into ui/compile-flags/invalid/ and ui/compile-flags/run-pass/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like type check something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to tests/ui/typeck/osstring-str-equality.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anything else resolve related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep moved to tests/ui/resolve/module-used-as-struct-constructor.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it more pattern directory, or even shadowing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I moved it to the pattern directory. and the test below moved it to the shadowing directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same as above but it looks like more shadowing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test wasn't moved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed it sorry
|
Everything else is fine For the tests amount I would say that 15+- is very comfortable to review If you want to do more that's fine as well, just might take more time but I have nothing against it |
I saw the comment left on another PR. I will change # to link! regex is awesome.. |
|
I'll take a last look before merge tomorrow, sorry for delay |
Note
Intermediate commits are intended to help review, but will be squashed add comment commit prior to merge.
part of #133895
tests/ui/compile-flagssplit it intotests/ui/compile-flags/invalid/andtests/ui/compile-flags/run-pass/r? Kivooeo