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

The API module probably MUST NOT depend on the oteltest which implements the API #2077

Closed
bogdandrutu opened this issue Jul 8, 2021 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@bogdandrutu
Copy link
Member

No description provided.

@bogdandrutu bogdandrutu added the bug Something isn't working label Jul 8, 2021
@MrAlias
Copy link
Contributor

MrAlias commented Jul 8, 2021

I'm not sure I follow. How can we test the API without a mock implementation?

@bogdandrutu
Copy link
Member Author

You have the noop for testing the API, and also tests in the SDK for testing the API implementation

@MrAlias
Copy link
Contributor

MrAlias commented Jul 9, 2021

You have the noop for testing the API, and also tests in the SDK for testing the API implementation

Allowing the noop implementation instead of the specific testing implementation we include in oteltest seems like an arbitrary and subjective choice. I'm following why that is allowed but oteltest is not. The oteltest package has no dependencies other than the github.com/stretchr/testify, go.opentelemetry.io/otel, and go.opentelemetry.io/otel/trace making it a concise way to test API implementations without including anything other than the API and the testify package.

This package is used in the otel package in the following places:

  • ./internal/global/trace_test.go
  • ./internal/global/propagator_test.go
  • ./propagation/trace_context_test.go
  • ./propagation/trace_context_benchmark_test.go

Each use would not be supported by the noop implementation as it does not provide the necessary testing framework. Meaning the existing testing utilities provided by the oteltest package would have to be duplicated in each package to provide similar testing capabilities. This is not ideal as it would require duplicate code and would not reduce the dependency load given the dependency breakdown mentioned above.

That said, it might be possible to refactor the oteltest package to accomplish this. We could abstract the parts we depend on in the otel package into an internal package and then export them with the oteltest package.

I'm not convinced this second approach is going to be worth it. I'm guessing it will add a lot of complication and indirection that are ultimately are not necessary only to remove this dependency. But it might be worth exploring to validate this assumption.

@bogdandrutu
Copy link
Member Author

Everything you say here I feel is not as important as the fact that right now I have 2 implementations of the API, and when unittesting my application (integration) I am testing it with a mock that is 90% copy paste from the SDK (but still duplicate code that has bugs, see the #2075), and my code can work great with that but not with the official SDK.

I think it is more important to put the user first, and ensure that their tests/unit-tests are testing the right thing and give them the confidence they need, instead of our tests. If we have to duplicate code for our tests that is much better than having this duplicate code be used by all integrations to do their tests.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 22, 2021

Closing as #2111 completed the dependency on the oteltest package. I'm going to open another issue to track the other concerns raised here: the surface area of the oteltest package should be minimized and if it provides an implementation of the SDK it needs to be as similar a code path as the production version.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 22, 2021

#2121

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
Archived in project
Development

No branches or pull requests

2 participants