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: wrong test/assertion in test-trace-http #615

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

jinwoo
Copy link
Contributor

@jinwoo jinwoo commented Nov 29, 2017

The wrong test is 'should not trace api requests'. The header name to
check should be TRACE_AGENT_REQUEST_HEADER, not
TRACE_CONTEXT_HEADER_NAME. Also the assertion should be done against the
number of spans, not against the number of traces.

Also fix the order of arguments to assert.equal() calls. The first
argument must be the actual result and the second the expected value.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 29, 2017
@jinwoo jinwoo requested a review from kjin November 29, 2017 19:05
@jinwoo
Copy link
Contributor Author

jinwoo commented Nov 29, 2017

CI failures seem like a flakiness. @kjin can you rerun Appveyor?

The wrong test is 'should not trace api requests'. The header name to
check should be TRACE_AGENT_REQUEST_HEADER, not
TRACE_CONTEXT_HEADER_NAME. Also the assertion should be done against the
number of spans, not against the number of traces.

Also fix the order of arguments to `assert.equal()` calls. The first
argument must be the actual result and the second the expected value.
@jinwoo
Copy link
Contributor Author

jinwoo commented Nov 30, 2017

@kjin All tests passed now. Can you take a look?

@jinwoo jinwoo merged commit 582ed50 into googleapis:master Nov 30, 2017
@jinwoo jinwoo deleted the fix-tests branch November 30, 2017 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants