migrate service update integration tests from integration-cli to integration/service package#37492
Conversation
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
one minor issue but LGTM otherwise (also left a suggestion of a follow up) 👍
integration/service/update_test.go
Outdated
There was a problem hiding this comment.
can you add t.Helper() in this function?
There was a problem hiding this comment.
Thank you for the review. Fixed.
integration/service/update_test.go
Outdated
There was a problem hiding this comment.
Looks like these would be a good candidate for sub-tests. Could be done as a follow up (at least, I'd prefer it to be done in a separate commit, because currently it's easier to compare/follow the "old" and "new" test 😅)
There was a problem hiding this comment.
Thank you for the review, I will follow up with a PR to refactor.
integration/service/update_test.go
Outdated
There was a problem hiding this comment.
This was because it was previously // +build !windows
…rvice_update_test.go to integration/service Signed-off-by: Arash Deshmeh <[email protected]>
adshmh
force-pushed
the
migrate-docker_cli_service_update_test-to-integration-service
branch
from
2d9231e to
fbaef1b
Compare
Codecov Report
@@ Coverage Diff @@
## master #37492 +/- ##
==========================================
+ Coverage 34.95% 35.54% +0.58%
==========================================
Files 610 610
Lines 44886 45113 +227
==========================================
+ Hits 15690 16034 +344
+ Misses 27077 26915 -162
- Partials 2119 2164 +45 |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thanks!
ping @vdemeester
|
lol; browser-cache; didn't see your review, vincent 😂 |
|
@thaJeztah I am not sure about this, but I think 2 of the tests may be flaky (don't know the reason yet), as they passed once and failed once (I could not reproduce any failures of those tests locally) |
|
@adshmh thanks for the heads-up; if you see them fail more, could you open an issue for tracking that flakiness? |
|
Yeah it's definitely flakey 😓 @adshmh I'll prepare a revert in the meantime to get the CI a bit more stable 😅 |
|
I will look into the reason for flakiness and submit the PR again after fixing the flaky tests. |
|
Thanks @adshmh! |
5 participants
- What I did
Refactored and moved service update integration tests from the
integration-clipackage tointegration/service- How I did it
Removed the integration test file
integration-cli//docker_cli_service_update_test.goand added new test fileintegration/service/update_test.gowith corresponding tests.- How to verify it
- Description for the changelog
Service update integration tests have been moved from integration-cli to integration/service package.
- A picture of a cute animal (not mandatory but encouraged)