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

Fix race condition in test, remove hard-coded namespace #382

Merged
merged 3 commits into from
May 3, 2019
Merged

Fix race condition in test, remove hard-coded namespace #382

merged 3 commits into from
May 3, 2019

Conversation

kevinearls
Copy link
Contributor

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

This fixes #380 where the test would fail if we did the http get before the span got flushed. I've also removed the hardcoded namespace of "default" from the es_index_cleaner_test.

Signed-off-by: Kevin Earls <kearls@redhat.com>
@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #382   +/-   ##
=======================================
  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 9ff5bb7...3999dbc. Read the comment docs.

@kevinearls
Copy link
Contributor Author

@pavolloffay @objectiser @jpkrohling Please review

if !strings.Contains(bodyString, "errors\":null") {
if (strings.Contains(bodyString, tStr)) {
return true, nil
} else if !strings.Contains(bodyString, "errors\":null") {
Copy link
Member

Choose a reason for hiding this comment

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

Keep the errors check separate from the expected spas and move it above it

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 Are you ok with these changes?

Signed-off-by: Kevin Earls <kearls@redhat.com>
@@ -45,10 +45,10 @@ func SmokeTest(apiTracesEndpoint, collectorEndpoint, serviceName string, interva

if !strings.Contains(bodyString, "errors\":null") {
Copy link
Member

Choose a reason for hiding this comment

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

Can be written as

if !strings.Contains(bodyString, "errors\":null") {
			return false, errors.New("query service returns errors")
		} 

return  strings.Contains(bodyString, tStr)), nil

Signed-off-by: Kevin Earls <kearls@redhat.com>
@pavolloffay pavolloffay merged commit 945569d into jaegertracing:master May 3, 2019
@kevinearls kevinearls deleted the issue380 branch May 3, 2019 10:55
@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.

Fix race condition in e2e SmokeTest
2 participants