-
Notifications
You must be signed in to change notification settings - Fork 586
Fix unstable integration tests. #1060
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
Conversation
Makefile
Outdated
| $(SWIFT) test -c $(BUILD_CONFIGURATION) $(INTEGRATION_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIVolumes || exit_code=1 ; \ | ||
| $(SWIFT) test -c $(BUILD_CONFIGURATION) $(INTEGRATION_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIKernelSet || exit_code=1 ; \ | ||
| $(SWIFT) test -c $(BUILD_CONFIGURATION) $(INTEGRATION_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIAnonymousVolumes || exit_code=1 ; \ | ||
| $(SWIFT) test -c $(BUILD_CONFIGURATION) $(INTEGRATION_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLINoParallelCases || exit_code=1 ; \ |
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.
The name suggests these tests should not run in parallel
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.
They are designed to run in parallel, and we do so to stress test the system. Unfortunately it appears that we have a suite that contains so many container run tests that the API server gets overloaded and XPC calls start timing out.
| @Test func testImageSingleConcurrentDownload() throws { | ||
| // removing this image during parallel tests breaks stuff! | ||
| _ = try? run(arguments: ["image", "rm", alpine]) | ||
| defer { _ = try? run(arguments: ["image", "rm", "--all"]) } |
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.
Consider adding logging in case of failures
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.
It's a good idea but I think we'd want to build that into our test support code. We put together the integration tests and related support code early on and there's plenty of room for refinement. It's out of scope for this PR as we just need to get tests working again, but consider filing an issue describing the enhancement, and then knock out a PR!
7db92e6 to
39ef497
Compare
- TestCLIRunCommand now run so many tests concurrently that the API server gets swamped and tests randomly time out. - The parallelism options on `swift test` only work for XCTest, not swift-testing. - Work around this while retaining some parallelism (good for stress testing) by breaking the tests into two suites.
ee667ca to
fae539c
Compare
swift testonly work for XCTest, not swift-testing.Type of Change
Motivation and Context
Most PRBs are breaking right now.
Testing