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

Make sure to call ctx.cleanup if prepare() fails #389

Merged
merged 1 commit into from
May 3, 2019
Merged

Make sure to call ctx.cleanup if prepare() fails #389

merged 1 commit into from
May 3, 2019

Conversation

kevinearls
Copy link
Contributor

Signed-off-by: Kevin Earls kearls@redhat.com

This fixed #388 Currently if tests fail during the prepare() call ctx.cleanup() will not be called and the namespace created for the test does not get cleaned up.

Signed-off-by: Kevin Earls <kearls@redhat.com>
@@ -15,7 +16,11 @@ import (

// Cassandra runs a test with Cassandra as the backing storage
func Cassandra(t *testing.T) {
ctx := prepare(t)
ctx, err := prepare(t)
Copy link
Member

Choose a reason for hiding this comment

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

Could we call prepare in the caller so each test would not have to repeat this block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavolloffay Once I finish the other changes I am making for reporting it will be called in the Setup method, only once per suite. https://github.com/jaegertracing/jaeger-operator/blob/master/test/e2e/all_in_one_test.go#L36-L44

I've run into this bug a few times though, so I wanted to get this done first.

@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #389 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #389   +/-   ##
=======================================
  Coverage   89.72%   89.72%           
=======================================
  Files          64       64           
  Lines        3094     3094           
=======================================
  Hits         2776     2776           
  Misses        216      216           
  Partials      102      102

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 945569d...75ffd43. Read the comment docs.

@kevinearls kevinearls changed the title Make sure to call ctx.cleanup if perpare() fails Make sure to call ctx.cleanup if prepare() fails May 3, 2019
@kevinearls
Copy link
Contributor Author

@pavolloffay @objectiser @jpkrohling Please review.

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

As short term workaround it is fine.

@objectiser objectiser merged commit 9882892 into jaegertracing:master May 3, 2019
@kevinearls kevinearls deleted the issue388 branch May 3, 2019 15:54
@pavolloffay pavolloffay added the bug Something isn't working label May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

e2e tests don't clean up if they fail during operator deploy
3 participants