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

Add regression tests to close some issues #59535

Merged
merged 5 commits into from
Jun 23, 2022

Conversation

AnthonyLatsis
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis commented Jun 17, 2022

No luck finding existing test cases for these issues.

Closes #42790, closes #52883, closes #59572, closes #43069, closes #43070

@AnthonyLatsis AnthonyLatsis requested a review from xedin June 17, 2022 18:43
@@ -351,3 +351,11 @@ func usesProtoRefinesClass2<T : ProtoRefinesClassComposition>(_ t: T) {
t.genericMethod((1, 2))
let _: BaseProto = t
}

class SR10483_C: SR10483_P {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s okay to use the SR in the names IMHO just add a link to github issue as a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, let us rename these to "issueNNNN", and I will add the links too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or some more neutral names would work too if you can come up with good ones. I usually avoid suggesting renaming in tests since I often struggle with that myself :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"issueNNNN" is IMHO the best one I could come up with as of now. I think "issue" is pretty neutral, in the sense that it need not apply just to bugs, and perhaps better than an abbreviation that is often hard to decipher and more strongly tied to a specific tracking system (like, I still wonder what SR stands for, Swift Radar? 😅).

@hborla What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I usually do is wrap everything in a func rdarXXX or func issueYYY and then no need to add anything to the names, but it doesn’t work if you need to declare a protocol or extensions…

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I misunderstood! Yes, I will switch to foo or something for the stuff that I could wrap in a do block—that was an accident. No need for a named wrapper in addition to the issue link, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I usually do is wrap everything in a func rdarXXX or func issueYYY and then no need to add anything to the names, but it doesn’t work if you need to declare a protocol or extensions…

Yeah I like this strategy too! Lately I've been using it but with more descriptive names (e.g. what the actual issue is) instead of an issue/jira/radar number. An issue link in a comment is perfectly sufficient if the issue has helpful context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I like this strategy too! Lately I've been using it but with more descriptive names (e.g. what the actual issue is) instead of an issue/jira/radar number. An issue link in a comment is perfectly sufficient if the issue has helpful context.

Me and Pavel misunderstood each other—I was referring to the "naming convention" that is to replace "SRXXX" , which I proposed to be "issueYYY". But I totally concur that wrapping issue-specific test cases in functions/blocks when possible and mentioning the issue once is a lot neater that sticking the ID in every identifier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did understand what you meant and issueXXX is fine, just like we do for rdarYYY and I should have clarified that my suggestion is separate, sorry!

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please clean smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@xedin Thank you for reviewing!

@AnthonyLatsis AnthonyLatsis merged commit 15acfa9 into apple:main Jun 23, 2022
@AnthonyLatsis AnthonyLatsis deleted the close-issues-1 branch June 23, 2022 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment