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

[SR-15353] tweak testSerialConcurrentPerform #13

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

QuietMisdreavus
Copy link
Contributor

Bug/issue #, if applicable: SR-15353

Summary

This PR is meant to help debug SR-15353 in CI, to figure out why the testSerialConcurrentPerform test is periodically failing on one of the Linux builders. Right now, it just tweaks the assertion logging, but i have a hunch as to what's going on (resolution/precision issues in the systemUptime clock) that i'll implement when i have a better picture of what's going wrong on the CI machine. (I couldn't reproduce it locally, whether in macOS or in a Linux Docker container.)

Dependencies

N/A

Testing

This PR updates an existing test and doesn't touch any library code; behavior should not change.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • [ n/a ] Added tests
  • Ran the ./bin/test script and it succeeded
  • [ n/a ] Updated documentation if necessary

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test Linux

}),
"Blocks didn't run serially"
)
for (result, next) in zip(results, results.dropFirst()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be really surprised if that changes anything. By the time this code runs all the results are already finalized - this code is just comparing them. I'd rather save the results into a log any time this assert fails on Linux and try to examine the logs to investigate why the test is failing. Just my 2¢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't intending on fixing the test with this change, just have more visibility into what's failing and how. The actual fix will come once i can get CI to run it 😉

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test Linux

3 similar comments
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test Linux

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test Linux

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test Linux

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci test

@QuietMisdreavus
Copy link
Contributor Author

It looks like the failure was because the loop was too fast, and caused some "start" timestamps to be identical to the previous "end" timestamp. I've updated the test to allow this; i'll run CI a few more times before marking the PR as ready.

@swift-ci test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci test Linux

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

2 similar comments
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci test

1 similar comment
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci test

1 similar comment
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci test

@QuietMisdreavus QuietMisdreavus marked this pull request as ready for review October 29, 2021 14:27
@QuietMisdreavus
Copy link
Contributor Author

So we've now hit 10 test runs without an issue, so i think we're in the clear. I'll call this ready to go now.

Copy link
Member

@franklinsch franklinsch left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@QuietMisdreavus QuietMisdreavus changed the title [WIP] [SR-15353] tweak testSerialConcurrentPerform [SR-15353] tweak testSerialConcurrentPerform Oct 29, 2021
@QuietMisdreavus QuietMisdreavus merged commit ba39315 into apple:main Oct 29, 2021
@QuietMisdreavus QuietMisdreavus deleted the serial-concurrent branch October 29, 2021 14:33
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.

None yet

3 participants