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

Add the test command to the dev loop #5190

Merged
merged 4 commits into from
Jan 7, 2021

Conversation

PriyaModali
Copy link
Contributor

Fixes: #5149, #5167

Description

Split build and test into separate functions
Add test command to dev loop

Note:
The new workflow is exercised by the existing unit tests and integration tests. No need to add new tests.

@codecov
Copy link

codecov bot commented Dec 25, 2020

Codecov Report

Merging #5190 (3a7bf31) into master (35214eb) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5190      +/-   ##
==========================================
+ Coverage   71.82%   71.86%   +0.04%     
==========================================
  Files         387      387              
  Lines       13928    13935       +7     
==========================================
+ Hits        10004    10015      +11     
+ Misses       3190     3188       -2     
+ Partials      734      732       -2     
Impacted Files Coverage Δ
pkg/skaffold/runner/build_deploy.go 68.88% <ø> (-1.01%) ⬇️
pkg/skaffold/runner/dev.go 71.52% <100.00%> (+2.01%) ⬆️
pkg/skaffold/util/tar.go 56.00% <0.00%> (+5.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35214eb...3a7bf31. Read the comment docs.

@PriyaModali PriyaModali changed the title Testing test in the dev loop Add the test command to the dev loop Dec 28, 2020
@PriyaModali PriyaModali marked this pull request as ready for review January 5, 2021 18:11
@PriyaModali PriyaModali requested a review from a team as a code owner January 5, 2021 18:11
@tete17
Copy link
Contributor

tete17 commented May 20, 2021

Hey @nkubala & @PriyaModali I am not entirely sure if this is the case but I believe this MR has introduced a regression.

In particular from 1.18 and forward the command skaffold build doesn't run the test anymore. In previous versions it used to so I am not sure how intentional this is. Here are the reasons I am a bit confused:

  • The help page for skaffold build --help still lists the --skip-tests as an option.
  • The documentation https://skaffold.dev/docs/pipeline-stages/testers/structure/ (on the right corner) mentioned the build command is not applicable, yet the example shown in the page calls to run skaffold build --profile quickcheck

We use to depend on this useful behaviour (for us) to simply run skaffold build on our CI and we would know for sure our images would be tested. I understand if this was a deliberate move to change the behaviour, I just want to have confirmation this is not a regression.

@briandealwis
Copy link
Member

Hi @tete17. We did split out test into a separate command, skaffold test. Thanks for pointing out the doc inconsistencies — I'll fix that now.

--skip-tests still makes sense for build as some builders, like Jib for Maven/Gradle, do run unit tests as part of their build. We should expose this to custom builders too, so I opened #5909.

@tete17
Copy link
Contributor

tete17 commented May 27, 2021

Hi @briandealwis thanks for the quick response.

I am sorry I should have also mentioned that when running skaffold build --help I get

msacristan@Miguel-ThinkPad-T460s:~$ skaffold build --help
Build, test and tag the artifacts
...

Another thing I would like to add is that I couldn't find a clear description in the releases of this new behaviour (that test would not be run during build). Maybe I missed it somewhere.

I appreciate the quick and awesome help you guys always provide

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split build and test into separate functions
4 participants