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

SpanAssert method names should reflect underlying Span method names #1307

Merged

Conversation

imavroukakis
Copy link
Contributor

Closes #690

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 2, 2020

CLA Check
The committers are authorized under a signed CLA.

@anuraaga
Copy link
Contributor

anuraaga commented Oct 2, 2020

Thanks @imavroukakis! This looks like the right change to the API but we need to update the usage in the tests too. It's groovy, but I think IntelliJ'a rename refactoring should automatically change them. Can you try that?

@imavroukakis
Copy link
Contributor Author

imavroukakis commented Oct 2, 2020

@anuraaga I'm making good progress but I'm a bit stumped by a couple of errors at GrpcStreamingTest and GrpcTest. The tests are complaining about a MissingMethodException e.g. at GrpcStreamingTest L117 but I can't see why that would be the case. Any suggestions would be appreciated

@iNikem
Copy link
Contributor

iNikem commented Oct 2, 2020

@imavroukakis Those tests have Spock data table which contribute input parameter name. This clashes with method called name. Maybe rename method to spanName?

@imavroukakis
Copy link
Contributor Author

@imavroukakis Those tests have Spock data table which contribute input parameter name. This clashes with method called name. Maybe rename method to spanName?

Thanks @iNikem, wouldn't have thought to look there, I've not used Spock before. Since the spirit of this PR is to have a method name equivalence between SpanAssert and the underlying Span would it make more sense to rename the Spock input parameter? If so, where should I look for it?

@imavroukakis
Copy link
Contributor Author

@imavroukakis Those tests have Spock data table which contribute input parameter name. This clashes with method called name. Maybe rename method to spanName?

Thanks @iNikem, wouldn't have thought to look there, I've not used Spock before. Since the spirit of this PR is to have a method name equivalence between SpanAssert and the underlying Span would it make more sense to rename the Spock input parameter? If so, where should I look for it?

ignore the comment about looking for the data table, I looked it up on the Spock documentation :)

@imavroukakis
Copy link
Contributor Author

thanks, @iNikem that got me a lot further! I've only got to figure out why the couchbase-2.6 tests are failing now.

@imavroukakis
Copy link
Contributor Author

Tests passed and the build is green https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/1307/checks?check_run_id=1199938449

I've done a force push after a rebase to tidy up the commits, @anuraaga feel free to review 😄

@trask
Copy link
Member

trask commented Oct 4, 2020

hey @imavroukakis! looks like we've hit the jfrog snapshot retention issue again. I opened #1320 to track this, I'll ping back on this thread once that's fixed so you can merge master.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks a lot, nice change

@trask
Copy link
Member

trask commented Oct 5, 2020

@imavroukakis master is fixed now 👍

@anuraaga
Copy link
Contributor

anuraaga commented Oct 5, 2020

Hmm CircleCI didn't run - let me try kicking it

@anuraaga anuraaga closed this Oct 5, 2020
@anuraaga anuraaga reopened this Oct 5, 2020
@anuraaga anuraaga merged commit f2de47a into open-telemetry:master Oct 5, 2020
@anuraaga
Copy link
Contributor

anuraaga commented Oct 5, 2020

Thanks @imavroukakis!

@imavroukakis imavroukakis deleted the update-spanAssert-method-names branch October 5, 2020 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update SpanAssert method names to reflect the underlying Span method names
5 participants