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

Fix SendToRecvFrom_Datagram_UDP, organize SendReceive tests #31878

Closed
wants to merge 3 commits into from
Closed

Fix SendToRecvFrom_Datagram_UDP, organize SendReceive tests #31878

wants to merge 3 commits into from

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Feb 6, 2020 •
edited
Loading

  1. Fix Test: System.Net.Sockets.Tests.SendReceiveEap.SendToRecvFrom_Datagram_UDP(loopbackAddress: ::1) failed in CI #1712 by moving those tests to the NoParallelTests collection. +also move them back to Inner Loop
  2. Organize SendReceive tests into explicit semantical groups, by using nested classes. If it does not get accepted, I will redo 1. with the old naming scheme, but I believe it will hurt readability and maintainability a lot, since the total number of classes is now doubled because of SendReceive_Socket_NonParallel<T> and derived classes.

@antonfirsov antonfirsov added area-System.Net.Sockets test-enhancement Improvements of test source code labels Feb 6, 2020
@antonfirsov antonfirsov added this to the 5.0 milestone Feb 6, 2020
@antonfirsov antonfirsov requested review from a team and davidsh February 6, 2020 19:19
@antonfirsov
Copy link
Member Author

This is how it looks in VS test explorer, the logic is immediately obvious IMO:
image

wfurt
wfurt approved these changes Feb 6, 2020
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.

@scalablecory scalablecory added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 7, 2020
@scalablecory
Copy link
Contributor

marking no-merge as we're waiting for discussion

@scalablecory
Copy link
Contributor

I don't know if there's enough organizational benefit versus using underscores here. I don't like the increased indent it adds to everything as you get deeper in the hierarchy.

@stephentoub
Copy link
Member

@antonfirsov, based on offline discussions, should this be closed? Seems like part (1) of the change is still relevant?

@antonfirsov
Copy link
Member Author

I will reopen a new PR for (1) when I'll have the time.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wfurt wfurt wfurt approved these changes

@davidsh davidsh Awaiting requested review from davidsh

Assignees
No one assigned
Labels
area-System.Net.Sockets NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) test-enhancement Improvements of test source code
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

Test: System.Net.Sockets.Tests.SendReceiveEap.SendToRecvFrom_Datagram_UDP(loopbackAddress: ::1) failed in CI
4 participants
@antonfirsov @scalablecory @stephentoub @wfurt