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.

Sign up for GitHub

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

Jump to bottom

Run ChannelCrashTests on Linux/CI #379

Merged
merged 1 commit into from
Feb 25, 2019
Merged

Run ChannelCrashTests on Linux/CI #379

merged 1 commit into from
Feb 25, 2019

Conversation

rebello95
Copy link
Collaborator

When ChannelCrashTests was added, it was never configured to run on Linux/CI. This PR:

  • Renames the test case to ChannelConnectivityTests
  • Adds an entry to LinuxMain so this is run on CI
  • Makes the class final
  • Updates indentation to be consistent with the rest of the project (2 spaces)

@rebello95
Run ChannelCrashTests on Linux/CI
72c407b 
When `ChannelCrashTests` was added, it was never configured to run on Linux/CI. This PR:

- Renames the test case to `ChannelConnectivityTests`
- Adds an entry to `LinuxMain` so this is run on CI
- Makes the class `final`
- Updates indentation to be consistent with the rest of the project (2 spaces)
@rebello95 rebello95 requested a review from MrMage February 24, 2019 20:51
MrMage
MrMage approved these changes Feb 25, 2019
View reviewed changes
Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Thank you for this housekeeping!

FYI, I don't think making test classes final has much benefit — it might actually be surprising given that this doesn't seem to be a common pattern in Swift. I guess it doesn't make much difference either way.

@rebello95
Copy link
Collaborator Author

My primary reasoning behind updating it to final is to make it clearer to contributors as to which test cases are designed to be subclassed in this project. As-is, not all of the test cases are designed to be subclassed, and I think this helps make the distinction a bit more obvious

@rebello95 rebello95 merged commit b899a30 into grpc:master Feb 25, 2019
@rebello95 rebello95 deleted the connectivity-tests branch February 25, 2019 15:00
@MrMage
Copy link
Collaborator

MrMage commented Feb 25, 2019

My primary reasoning behind updating it to final is to make it clearer to contributors as to which test cases are designed to be subclassed in this project. As-is, not all of the test cases are designed to be subclassed, and I think this helps make the distinction a bit more obvious

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrMage MrMage MrMage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rebello95 @MrMage