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

Refactor: reduce length of issue-72933 and issue-74564 test #82075

Closed

Conversation

Skgland
Copy link
Contributor

@Skgland Skgland commented Feb 13, 2021

Part of #60302

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2021
@nagisa
Copy link
Member

nagisa commented Feb 14, 2021

Do these new tests still reproduce the original issues properly? Please verify and mention this in the commit/PR description.

@Mark-Simulacrum
Copy link
Member

I would also say - I'm not sure how much it matters for tests whether they are long or short, but rather I'd optimize for retaining clarity. I feel like adding a macro doesn't really do that for me, rather, makes it harder to understand the test...

@Skgland
Copy link
Contributor Author

Skgland commented Feb 14, 2021

Do these new tests still reproduce the original issues properly? Please verify and mention this in the commit/PR description.

Yes, I made sure they still reproduced with the Version that was mentioned in the Issue to have the regression

For Issue 74564 this was done using stable 1.46.0 and for issue 72933 this was done with the 1.45.0 nightly 2020-06-01

@Skgland
Copy link
Contributor Author

Skgland commented Feb 14, 2021

I would also say - I'm not sure how much it matters for tests whether they are long or short, but rather I'd optimize for retaining clarity. I feel like adding a macro doesn't really do that for me, rather, makes it harder to understand the test...

That sounds reasonable, in that case these files should be removed from #60302 (comment) and this PR can simply be closed. Alternatively I could try adding a comment to explain what is being done/tested.

@Mark-Simulacrum
Copy link
Member

Yeah, I think I'd prefer to just not do this.

@Skgland Skgland deleted the reduce_some_test_for_issue_60302 branch February 22, 2021 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants