-
Notifications
You must be signed in to change notification settings - Fork 623
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
Rename base test assertion methods for consistency #2084
Conversation
6321479
to
3ed70c5
Compare
3ed70c5
to
53c5b48
Compare
53c5b48
to
a63dec4
Compare
@@ -44,11 +46,11 @@ def tearDownClass(cls): | |||
def setUp(self): | |||
self.memory_exporter.clear() | |||
|
|||
def check_span_instrumentation_info(self, span, module): | |||
def assertSpanInstrumentationInfo(self, span, module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned this in the contrib repo PR, but I would prefer the name assertSpanHasInstrumentationInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
a63dec4
to
bdd96bf
Compare
bdd96bf
to
9957a51
Compare
@@ -44,11 +46,11 @@ def tearDownClass(cls): | |||
def setUp(self): | |||
self.memory_exporter.clear() | |||
|
|||
def check_span_instrumentation_info(self, span, module): | |||
def assertSpanHasInstrumentationInfo(self, span, module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not trying to be nitpicky but this name is misleading. It is not checking that the span has instrumentation info but that the span instrumentation info is the same as the one in the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe assertEqualSpanModuleInstrumentationInfo
? 🤷
I'm merging before other approvers want more renames. 😂 |
Description
Update base test class to have more consistent assertion methods with rest of Python ecosystem.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: