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

Add reset for configuration testing #636

Merged
merged 5 commits into from
May 5, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented May 1, 2020

Fixes #634

@ocelotl ocelotl requested a review from a team May 1, 2020 20:11
@ocelotl ocelotl added api Affects the API package. enhancement New feature or request tests labels May 1, 2020
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@ocelotl ocelotl self-assigned this May 4, 2020
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit.

Should this include a test for _reset as well?

Comment on lines +131 to +132
It is not intended to be used by production code but by testing code
only.

Choose a reason for hiding this comment

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

Suggested change
It is not intended to be used by production code but by testing code
only.
It is intended to be used by testing code only.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Switching mine to request changes, as a test is really important.

@ocelotl
Copy link
Contributor Author

ocelotl commented May 5, 2020

Added the test you requested, @toumorokoshi 👍

@ocelotl ocelotl requested a review from toumorokoshi May 5, 2020 18:44
@toumorokoshi
Copy link
Member

@ocelotl one last comment / discussion, thanks!

@ocelotl ocelotl requested a review from toumorokoshi May 5, 2020 18:55
@toumorokoshi toumorokoshi merged commit b5b297c into open-telemetry:master May 5, 2020
@ocelotl ocelotl deleted the issue_634 branch May 5, 2020 19:51
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
…-telemetry#636 (open-telemetry#654)

* feat: warn user when a instrumented package was already required open-telemetry#636

* chore: address PR comments

* chore: extract package name from require.cache

* chore: use find instead of some

* chore: use require.resolve to find already required modules

* chore: try/catch require.resolve

Co-authored-by: Mayur Kale <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package. enhancement New feature or request tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a standard way to "reset" a Configuration object for testing
4 participants