Skip to content

Review tests in tests/ui/numbers-arithmetic for duplicates #119851

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

Closed
RalfJung opened this issue Jan 11, 2024 · 8 comments · Fixed by #121432
Closed

Review tests in tests/ui/numbers-arithmetic for duplicates #119851

RalfJung opened this issue Jan 11, 2024 · 8 comments · Fixed by #121432
Assignees
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-help-wanted Call for participation: Help is requested to fix this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jan 11, 2024

#119432 adds extensive tests for our overflow lint; as far as I can see they subsume most of what we have in tests/ui/numbers-arithmetic, so the old tests can be deleted. Yay for having the tests nicely grouped in one file rather than spread across a dozen files!

So, once #119432 lands, we should do a review of these tests and remove duplicates.

Cc @gurry

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 11, 2024
@fmease fmease added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-help-wanted Call for participation: Help is requested to fix this issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 11, 2024
@mj10021
Copy link
Contributor

mj10021 commented Jan 11, 2024

@rustbot claim

@mj10021
Copy link
Contributor

mj10021 commented Jan 15, 2024

@RalfJung if there are some tests in numbers-arithmetic that are not exactly represented in the new tests, should I just add them to the new test file once its merged to clean up as much as possible?

@RalfJung
Copy link
Member Author

I think that depends on whether they would fit the existing test files or not. The first step would be to determine whether there are any such tests.

@mj10021
Copy link
Contributor

mj10021 commented Feb 1, 2024

hey any idea when the PR will get merged? it will be a lot easier to reorganize the tests then

@RalfJung
Copy link
Member Author

RalfJung commented Feb 2, 2024

It's waiting on a team decision. The teams are busy so it's hard to predict when this will get to the top of their queue.

@RalfJung
Copy link
Member Author

RalfJung commented Feb 17, 2024

#119432 has landed, so the review and cleanup work can commence. :) @mj10021 are you still planning to take a look at this?

@mj10021
Copy link
Contributor

mj10021 commented Feb 17, 2024

#119432 has landed, so the review and cleanup work can commence. :) @mj10021 are you still planning to take a look at this?

Oh, yes definitely, thanks for the update. I'm going away for a week, so I will start today but might not be able to finish until I get back.

@RalfJung
Copy link
Member Author

No worries, this is not urgent. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-help-wanted Call for participation: Help is requested to fix this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants