Some slight tweaks for the integration test#37086
Some slight tweaks for the integration test#37086anusha-ragunathan merged 1 commit intomoby:masterfrom
Conversation
There was a problem hiding this comment.
08:14:51 integration/internal/swarm/service.go:22:23:warning: Minutes not declared by package time (unconvert)
08:14:51 integration/internal/swarm/service.go:22:23:warning: Minutes not declared by package time (interfacer)
08:14:51 integration/internal/swarm/service.go:22:23:warning: Minutes not declared by package time (gosimple)
Pretty sure it's Minute :stuck_out_tongue_closed_eyes:
There was a problem hiding this comment.
Yeal, you're right, is forcing push the update 😆
arm64b
force-pushed
the
some-tweaks-for-CI-tests
branch
from
72b3907 to
6da1c65
Compare
There was a problem hiding this comment.
Should we revert the other change now? (trimming)?
There was a problem hiding this comment.
Trimming doesn't matter since we only observed \x00 in the output in terms of the test case, I think if we revert the trimming and the issue doesn't show in the future, that will be a strong evidence for the root cause(file I/O sync issue). OK, I will remove the trimming...
There was a problem hiding this comment.
Thanks, yes, that's what I was thinking: the test worked in the past (without trimming), so adding the trim could hide the bug in the test.
If it's still flaky, we could always decide to add it again 👍
There was a problem hiding this comment.
Unfortunately, looks like it's still flaky, so we may want to add that change again #36877 (comment)
arm64b
force-pushed
the
some-tweaks-for-CI-tests
branch
from
6da1c65 to
99199a6
Compare
Codecov Report
@@ Coverage Diff @@
## master #37086 +/- ##
=========================================
Coverage ? 35.03%
=========================================
Files ? 615
Lines ? 45821
Branches ? 0
=========================================
Hits ? 16052
Misses ? 27657
Partials ? 2112 |
There was a problem hiding this comment.
I'm not sure if this is the right approach. This PR increases the default poll timeout for services for ALL tests, which only TestServicePlugin is failing. This could mask real problems later.
Instead, how about an approach like this, specifically for the failing test? #36551
There was a problem hiding this comment.
@anusha-ragunathan Don't worry about the value for ALL tests to paper cover some issues, the rational here is: for a test case with convergent timeout value, timeout doesn't mean a defect of a function, because the duration depends on couple of factors like system performance, density of workload, scheduling.... Although this PR take TestServicePlugin as an example, but that's only a random case captured. Please see PR #36706, before that we tweak the timeout value case by case on AArch64.
Do you have info that some test cases have to be finished in a fixed duration?
There was a problem hiding this comment.
The problem is we have tests that were working fine which are now flakey. The reason changing this can cover up real issues is because it seems like the we have some slowdowns in the code and and extending the timeout could be covering up those slowdowns.
There was a problem hiding this comment.
OK, since we have some concerns about this (though I don't think the timeout failure will really help us some perf issue), let's remove this from this PR.
`arm64` needs get more time duration for the test to finish. `pty.Start()` opens a file, so the caller should close it explicitly, else the file I/O can result in unexpected data synchronization issue. All those changes will not affect the test itself. Signed-off-by: Dennis Chen <[email protected]>
arm64b
force-pushed
the
some-tweaks-for-CI-tests
branch
from
99199a6 to
476d787
Compare
|
Removed the concerned timeout code... Done |
6 participants
Service is time-consuming operation, we've observed some timeout failures for this kind of test such as TestServicePlugin, so we adjust the timeout value from '30s' to '1m' in this PR..
arm64needs get more time duration for the test to finish.pty.Start()opens a file, so the caller should close it explicitly, else the file I/O can result in unexpected data synchronization issue.All those changes will not affect the test itself.
Signed-off-by: Dennis Chen [email protected]
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)