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

Stop reusing same object to avoid beeing contaminated by previous test's operation #4615

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ami-GS
Copy link
Contributor

@ami-GS ami-GS commented Oct 15, 2024

Description

ConnectionPriority test has been failing frequently at stream start order validation.
Increase sleep time for actually flush any remaining operations in processing queue
Cleanup and recreate MsQuic Objects for later test not to be contaminated by previous test's operation

Issue description
ConnPriorityOpTestIssue.pdf

Testing

Works 1000 times locally without sleep.
Need double check with CI

Documentation

N/A

@ami-GS ami-GS marked this pull request as ready for review October 15, 2024 18:30
@ami-GS ami-GS requested a review from a team as a code owner October 15, 2024 18:30
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.72%. Comparing base (4316dbb) to head (9baef42).
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4615      +/-   ##
==========================================
- Coverage   87.02%   86.72%   -0.31%     
==========================================
  Files          56       56              
  Lines       17354    17361       +7     
==========================================
- Hits        15103    15056      -47     
- Misses       2251     2305      +54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -3656,7 +3656,7 @@ void QuicTestConnectionPriority()
// e.g. This test case randomly swap [3] and [4] and fail without sleep.
// This happens when [3] is already in the worker queue.
// Normal enqueue doesn't re-queue the Connection
CxPlatSleep(300);
CxPlatSleep(1000);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to make these event based? Is there some event we could wait on instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe possible by ingesting dummy Operation on dummy Connection

@ami-GS ami-GS changed the title Sleep longer to flush queue for sure Stop reusing same object to avoid beeing contaminated by previous test's operation Nov 29, 2024
@ami-GS
Copy link
Contributor Author

ami-GS commented Nov 30, 2024

Fixed only on Linux

@ami-GS ami-GS force-pushed the dev/daiki/prioritytest_flushqueue branch from 83e1616 to 9baef42 Compare November 30, 2024 21:41
@nibanks
Copy link
Member

nibanks commented Dec 2, 2024

Can you add more details/explanation of the core problem and how this fixes it?

@ami-GS
Copy link
Contributor Author

ami-GS commented Dec 2, 2024

Before actually test starts (issuing StreamStart/StreamSend), implicit connection operations are processed.
When any stream operation, which have to be processed later, is inserted to the implicitly processed connection, the expected StreamStart/StreamSend order is messed up.
The sleep was to flush all these implicit operations.
I thought GetParam can also flush all operation as it is blocking call, but there is still error. I believe that any of implicit operation can be still potentially issued after GetParam call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants