Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 poll watcher support to spec watcher for M1 #8449
Add poll watcher support to spec watcher for M1 #8449
Changes from 25 commits
c5f61ca
2c74e2d
98e2fe0
f98637f
d7ca9d6
2bf25f2
4c94362
74d08f2
cb50a8b
fefa482
d25e5d0
9d969c5
2645453
8b0d6af
0965fc4
83a255f
8f8d792
92e95db
22cae13
ad66078
6e20335
2d9ee6b
c701efc
1802518
9cec928
3498793
78dcafa
b5762e4
53a660f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you briefly describe the difference and how that affects those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description updated in the code:
// When using the PollWatcher variant of SupWatcher, the
// behavior is different than the NotifyWatcher. The NotifyWatcher
// receives event callbacks in response to a file or directory change,
// while the PollWatcher must poll and check for changes.
// As such, there were observed differences in the timing and number
// of events that the PollWatcher will handle. It was determined
// through analyzing the output that the poll watcher as written is not handling
// test cases correctly after the second test case. It does not receive
// the required events to pass the test case regardless of timing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you briefly describe the difference and how that affects the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets extract the common code from these 2 tests into a sharable function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on my buildkite and local tests, we can remove this sleep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on my buildkite and local tests, we can remove this sleep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
15 seconds seems really high. Did we play much with smaller delays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally got things working with a 30 second delay and then reduced it to 15 seconds and did not find it to be a problem. I can try. Will reduce if it will continue to pass the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update - 5 second sleep failed. 10 second sleep failed. Reverting to 15 second delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to bring this to 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to bring this to 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
15 seconds seems really high. Did we play much with smaller delays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally got things working with a 30 second delay and then reduced it to 15 seconds and did not find it to be a problem. I can try. Will reduce if it will continue to pass the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain why the iteration count needs to be bumped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Through experimentation it was determined that the PollWatcher is less responsive
and emits more events than the NotifyWatcher. The initial sleep used in NotifyWatcher
was not adequate to pass the tests and was increased as a result. Also the number of iterations
required is larger for the PollWatcher case as there were intermediate events observed that would lead to
test case failure with the original iteration count used. The test cases will fail if the desired events are not emitted, so the iteration count was increased to account for the increased number of events in the PollWatcher."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to make this 3 iterations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to make this 3 iterations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain why the iteration count needs to be bumped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Through experimentation it was determined that the PollWatcher is less responsive
and emits more events than the NotifyWatcher. The initial sleep used in NotifyWatcher
was not adequate to pass the tests and was increased as a result. Also the number of iterations
required is larger for the PollWatcher case as there were intermediate events observed that would lead to
test case failure with the original iteration count used. The test cases will fail if the desired events are not emitted, so the iteration count was increased to account for the increased number of events in the PollWatcher."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have both tests just look at the last one if that is the only one that matters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the reason for testing only the last element in the PollWatcher tests as it can't test the entire set of events in order as the NotifyWatcher can. That would be a degradation of the NotifyWatcher test. It does work though if you think we should add it as a test for NotifyWatcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have both tests just look at the last one if that is the only one that matters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate question