test(sdk-trace-base): ensure environment variables are cleaned up between tests #6011
Conversation
…ween tests The BasicTracerProvider tests currently clean up their modified environment variables at the end of each test. This is unreliable because if a given test fails, the clean up code is not run, and can leak environment variables into surrounding tests. This commit updates the BasicTracerProvider tests to ensure that modified environment variables are cleaned up between tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6011 +/- ##
=======================================
Coverage 95.00% 95.00%
=======================================
Files 313 313
Lines 8771 8771
Branches 1883 1883
=======================================
Hits 8333 8333
Misses 438 438 🚀 New features to boost your workflow:
|
david-luna
left a comment
david-luna
left a comment
There was a problem hiding this comment.
@cjihrig thanks for contributing :)
I left a comment. Let me know what you think.
| describe('constructor', () => { | ||
| describe('spanLimits', () => { | ||
| describe('when attribute value length limit is defined via env', () => { | ||
| afterEach(function () { |
There was a problem hiding this comment.
nit: when tests of this file are finished these env vars will be deleted but we are not certain if that was the env state before executing this file. following the spirit of this PR we could have a beforeEach block to stash the values in a couple of local vars and then restore these values in the afterEach
There was a problem hiding this comment.
Thanks for the review @david-luna. I'm fine with making whichever changes you want. I've noticed that environment variables in tests are handled inconsistently across the project (which is kind of expected in such a large project). For example:
- Some unconditionally delete any environment variables used by the tests in an
afterEach()hook. That is the style I copied here. - Some delete the environment variables at the end of the test body. This is probably the least good approach since the cleanup might never run if the test fails.
- Some make an effort to completely restore the environment. This is the most complete approach IMO, but does have its own drawbacks - see test(opentelemetry-configuration): preserve special process.env behavior #6010. This probably has the biggest performance hit as well, but probably is not an issue in practice. Also, if the approach from test(opentelemetry-configuration): preserve special process.env behavior #6010 were to be adopted across other packages, it would probably make sense to put it in some centralized place, but I'm not sure where those types of test helpers would live in the OTel project.
- There may be some other approaches I'm missing here.
Thoughts?
There was a problem hiding this comment.
On the examples you provided I think we should avoid the second option (deleting env vars at the end of the test). I also think the performance is not an issue but having to create a package or similar to share this logic across all packages for testing seems a bit overkill. A comment about it when there is a PR updating tests would gradually move the entire codebase to have the same approach regarding env.
I'd say let's go for your option and review it in a while :)
3 participants
Which problem is this PR solving?
The
BasicTracerProvidertests currently clean up their modified environment variables at the end of each test. This is unreliable because if a given test fails, the clean up code is not run, and can leak environment variables into surrounding tests.Fixes # N/A
Short description of the changes
This commit updates the
BasicTracerProvidertests to ensure that modified environment variables are cleaned up between tests.Type of change
How Has This Been Tested?
Checklist: