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

tests(fix): remove defer on tests #4272

Merged
merged 2 commits into from
May 6, 2024
Merged

Conversation

jLopezbarb
Copy link
Contributor

Proposed changes

Fixes Make explicit call to remove namespace. We have a bug that the defer was not executed on the last tests, leaving a namespace without deleting.

This change has one side effect. Now the tests that fails will not remove the ns that fails in order to look into it

CLI Quality Reminders 🔧

For both authors and reviewers:

  • Scrutinize for potential regressions
  • Ensure key automated tests are in place
  • Build the CLI and test using the validation steps
  • Assess Developer Experience impact (log messages, performances, etc)
  • If too broad, consider breaking into smaller PRs
  • Adhere to our code style and code review guidelines

Signed-off-by: Javier Lopez <javier@okteto.com>
@jLopezbarb jLopezbarb requested a review from a team as a code owner April 30, 2024 10:55
Copy link
Contributor

@maroshii maroshii left a comment

Choose a reason for hiding this comment

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

Now the tests that fails will not remove the ns that fails in order to look into it

I think this is acceptable

@andreafalzetti
Copy link
Contributor

@jLopezbarb I've added the label to run the e2e tests so we can try the change

@andreafalzetti andreafalzetti added the run-e2e When used on a PR run windows & unix e2e label Apr 30, 2024
Copy link
Contributor

@andreafalzetti andreafalzetti left a comment

Choose a reason for hiding this comment

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

Approving, just check the e2e tests if they pass before merging 🙏

Signed-off-by: Javier Lopez <javier@okteto.com>
Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.60%. Comparing base (1091b84) to head (f9d404a).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4272   +/-   ##
=======================================
  Coverage   45.60%   45.60%           
=======================================
  Files         308      308           
  Lines       27809    27809           
=======================================
  Hits        12682    12682           
  Misses      14035    14035           
  Partials     1092     1092           

@jLopezbarb jLopezbarb merged commit 95cddae into master May 6, 2024
14 checks passed
@jLopezbarb jLopezbarb deleted the jlo/remove-defer-on-tests branch May 6, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/bug-fix run-e2e When used on a PR run windows & unix e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants