8000 Add skipIfNoCache to tests by maroshii · Pull Request #4612 · okteto/okteto · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add skipIfNoCache to tests #4612

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 11, 2024
Merged

Add skipIfNoCache to tests #4612

merged 4 commits into from
Dec 11, 2024

Conversation

maroshii
Copy link
Contributor
@maroshii maroshii commented Dec 9, 2024

Restore functionality to skip tests if no cache is set. Depends on:

By defaults, cache will be disabled. To enabled cache for tests, users should add skipIfNoFileChanges=true to the/each section in the manifest:

test:
  unit:
    image: okteto/golang:1
    skipIfNoFileChanges: true
    commands:
      - "go test . -v"

@maroshii maroshii requested a review from a team as a code owner December 9, 2024 17:32
@maroshii
Copy link
Contributor Author
maroshii commented Dec 9, 2024

cc @viddo

@viddo
Copy link
Contributor
viddo commented Dec 10, 2024

Is there some way to skip/override the manifest setting through the CLI? like okteto --skipIfNoFileChanges=false I mean

ifbyol
ifbyol reviewed Dec 10, 2024
View reviewed changes
pkg/model/test.go
Caches []string `yaml:"caches,omitempty"`
Artifacts []Artifact `yaml:"artifacts,omitempty"`
Hosts []Host `yaml:"hosts,omitempty"`
SkipIfNoFileChanges bool `yaml:"skipIfNoFileChanges,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

We should include this field in the JSON schema. Ask @andreafalzetti if you need help to know how to do it

< 8000 span data-view-component="true">

@ifbyol
Copy link
Member
ifbyol commented Dec 10, 2024

A couple of questions:

  • Could you include an screenshot on how it looks like the output when the test is skipped?
  • Related to Nickla's questions, How does the --no-cache parameter affects to it?

@maroshii maroshii force-pushed the fran/ssh-build-secret branch from 0bffb78 to c3f706f Compare December 10, 2024 17:02
Base automatically changed from fran/ssh-build-secret to master December 10, 2024 17:09
@maroshii maroshii force-pushed the fran/skip-if-no-cache branch from 7454f5e to 4c67226 Compare December 10, 2024 17:10
@maroshii maroshii force-pushed the fran/skip-if-no-cache branch from 4c67226 to 9efb2e4 Compare December 10, 2024 17:12
@maroshii
Copy link
Contributor Author
  • Could you include an screenshot on how it looks like the output when the test is skipped?
Screenshot 2024-12-10 at 2 17 27 PM Screenshot 2024-12-10 at 2 17 16 PM
  • Related to Nickla's questions, How does the --no-cache parameter affects to it?

--no-cache should override the behavior and bust the cache even if the manifest sets it. That's what I would expect at least. Updated to condition to:

if test.SkipIfNoFileChanges && !options.NoCache {
	params.CacheInvalidationKey = "const"
}

@ifbyol
Copy link
Member
ifbyol commented Dec 11, 2024

I think you need to run the generate-schema command to update the local schema.json file. @andreafalzetti Should we document in the readme the process of updating the schema when changing something in the Okteto Manifest?

teresaromero
teresaromero reviewed Dec 11, 2024
View reviewed changes
pkg/remote/remote.go
8000
@@ -320,7 +326,7 @@ func (r *Runner) Run(ctx context.Context, params *Params) error {
outputMode = buildCmd.DeployOutputModeOnBuild
}

buildOptions := buildCmd.OptsFromBuildInfoForRemoteDeploy(buildInfo, &types.BuildOptions{OutputMode: outputMode, NoCache: params.NoCache})
buildOptions := buildCmd.OptsFromBuildInfoForRemoteDeploy(buildInfo, &types.BuildOptions{OutputMode: outputMode})
Copy link
Contributor

Choose a reason for hiding this comment

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

the cache option was not passed to the remote, was this intentionally? we do not use cache for remote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NoCache had no effect even if passed in. See the implementation of OptsFromBuildInfoForRemoteDeploy:

https://github.com/okteto/okteto/blob/master/pkg/cmd/build/build.go#L327-L335

@teresaromero
Copy link
Contributor

should we update integration test to cover this change? can we run integration with the current ones to see if affects?

@ifbyol
Copy link
Member
ifbyol commented Dec 11, 2024

I was testing it and it looks great!

@codyjlandstrom @pchico83 I have a doubt on how the cache should work when there is a depend_on. I will try to explain the scenario I refer to. Imagine I have the following manifest:

test:
  unit:
    context: unit/
    commands:
      - name: "Run unit tests"
        command: go test....

  integration:
    skipIfNoFileChanges: true
    context: integration/
    commands:
      - name: "Run integration tests"
         command: go test .....

If I execute okteto test, and unit is executed because it is not using cache or there was a change in its context, should integration run too even if there wasn't a change in the integration context because it depends on unit?

@pchico83
Copy link
Contributor

I was testing it and it looks great!

@codyjlandstrom @pchico83 I have a doubt on how the cache should work when there is a depend_on. I will try to explain the scenario I refer to. Imagine I have the following manifest:

test:
  unit:
    context: unit/
    commands:
      - name: "Run unit tests"
        command: go test....

  integration:
    skipIfNoFileChanges: true
    context: integration/
    commands:
      - name: "Run integration tests"
         command: go test .....

If I execute okteto test, and unit is executed because it is not using cache or there was a change in its context, should integration run too even if there wasn't a change in the integration context because it depends on unit?

I wouldn't tight skipIfNoFileChanges with dependencies until we have user feedback and clear use cases. It's always easier to add functionality than to remove it. In general, I expect skipIfNoFileChanges to be used on unit and deterministic tests without dependencies

@codecov Codecov
Copy link
codecov bot commented Dec 11, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 47.75%. Comparing base (3a409e6) to head (d258bd5).
Report is 4 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #4612   +/-   ##
=======================================
  Coverage   47.74%   47.75%           
=======================================
  Files         369      369           
  Lines       29712    29721    +9     
=======================================
+ Hits        14185    14192    +7     
- Misses      14428    14430    +2     
  Partials     1099     1099

@maroshii maroshii merged commit 38c7a19 into master Dec 11, 2024
12 of 13 checks passed
@maroshii maroshii deleted the fran/skip-if-no-cache branch December 11, 2024 12:44
@BrewTestBot BrewTestBot mentioned this pull request Jan 9, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teresaromero teresaromero teresaromero left review comments

@ifbyol ifbyol ifbyol approved these changes

Assignees
No one assigned
Labels
release/new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maroshii @viddo @ifbyol @teresaromero @pchico83
0