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

test: keep namespace when test failed in dev mod #1158

Merged
merged 6 commits into from
Jul 25, 2022

Conversation

stillfox-lee
Copy link
Contributor

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

Related issue
Add environment variable E2E_ENV to indicate e2e-test environment. If test case failed and E2E_ENV=dev, related resources will not be deleted.

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2022

Codecov Report

Merging #1158 (fde980e) into master (628abb9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1158   +/-   ##
=======================================
  Coverage   30.45%   30.45%           
=======================================
  Files          81       81           
  Lines        9839     9839           
=======================================
  Hits         2996     2996           
  Misses       6517     6517           
  Partials      326      326           

Help us with your feedback. Take ten seconds to tell us how you rate us.

@tao12345666333
Copy link
Member

LGTM

also cc @AlinsRan

@@ -442,6 +442,8 @@ func (s *Scaffold) beforeEach() {
func (s *Scaffold) afterEach() {
defer ginkgo.GinkgoRecover()

shouldKeepNamespace := false

if ginkgo.CurrentSpecReport().Failed() {
Copy link
Member

Choose a reason for hiding this comment

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

If we keep the namespace, we don't need to dump namespace contents anymore.

Copy link
Member

Choose a reason for hiding this comment

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

+1.

We can directly check the relevant information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is three any situation we still need real-time information from the namespace? Sometimes e2e-test may spend over an hour. Some resources like Event might be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

We do not currently dump this resource either.

@tao12345666333
Copy link
Member

I can help you resolve the conflicts. Just add one commit to delete the action.yml file

shouldDeleteNamespace := true
if ginkgo.CurrentSpecReport().Failed() && os.Getenv("E2E_ENV") == "dev" {
// Keep namespace if test failed in dev mode
shouldDeleteNamespace = false
Copy link
Member

Choose a reason for hiding this comment

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

We are not going to remove the dump feature cuz we need it to debug CI failures.
I mean, if the environment is dev, we can skip dump. When we are in CI environment, we still need dump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, my bad. Of course we need dump namespace info in CI. Thanks for your review!

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

I think the logic here could be simpler. I hope we only make relevant logical judgments through E2E_ENV=dev .

If in dev, we shouldn't delete namespace nor dump namespace resource.
Otherwise, delete namespace and dump namespace resource

@stillfox-lee
Copy link
Contributor Author

I think the logic here could be simpler.

Indeed. I can try to implement it in a simpler way. Thanks for your suggestion.

@tao12345666333
Copy link
Member

Thanks

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

LGTM

@tao12345666333 tao12345666333 merged commit 1765ec9 into apache:master Jul 25, 2022
@stillfox-lee stillfox-lee deleted the test/keep-namespace branch July 26, 2022 14:05
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.

None yet

4 participants