Skip to content

test(sdk-trace-base): ensure environment variables are cleaned up between tests #6011

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.

Sign up for GitHub

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

Merged
david-luna merged 3 commits into from
Oct 15, 2025
Merged

test(sdk-trace-base): ensure environment variables are cleaned up between tests #6011

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ab935c
test(sdk-trace-base): ensure environment variables are cleaned up bet…
cjihrig Oct 13, 2025
5ca23c2
Apply suggestion from @cjihrig
cjihrig Oct 13, 2025
71f3d0a
Merge branch 'main' into basic-trace-provider
david-luna Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2

* test(shim-opentracing): add comparison thresholds in flaky assertions [#5974](https://github.com/open-telemetry/opentelemetry-js/pull/5974) @cjihrig
* test(exporter-jaeger): clean up OTEL_EXPORTER_JAEGER_AGENT_PORT between tests [#6003](https://github.com/open-telemetry/opentelemetry-js/pull/6003) @cjihrig
* test(sdk-trace-base): ensure environment variables are cleaned up between tests [#xxxx](https://github.com/open-telemetry/opentelemetry-js/pull/xxxx) @cjihrig

## 2.1.0

Expand Down
18 changes: 10 additions & 8 deletions packages/opentelemetry-sdk-trace-base/test/node/BasicTracerProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,18 @@ describe('BasicTracerProvider - Node', () => {
describe('constructor', () => {
describe('spanLimits', () => {
describe('when attribute value length limit is defined via env', () => {
afterEach(function () {
Copy link
Contributor

@david-luna david-luna Oct 14, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cjihrig cjihrig Oct 14, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@david-luna david-luna Oct 15, 2025

Choose a reason for hiding this comment

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

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 :)

delete process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT;
delete process.env.OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT;
});

it('should have general attribute value length limits value as defined with env', () => {
process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT = '115';
const tracer = new BasicTracerProvider().getTracer(
'default'
) as Tracer;
const generalLimits = tracer.getGeneralLimits();
assert.strictEqual(generalLimits.attributeValueLengthLimit, 115);
delete process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT;
});
it('should have span attribute value length limit value same as general limit value', () => {
process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT = '125';
Expand All @@ -52,7 +56,6 @@ describe('BasicTracerProvider - Node', () => {
const spanLimits = tracer.getSpanLimits();
assert.strictEqual(generalLimits.attributeValueLengthLimit, 125);
assert.strictEqual(spanLimits.attributeValueLengthLimit, 125);
delete process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT;
});
it('should have span and general attribute value length limits as defined in env', () => {
process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT = '125';
Expand All @@ -64,20 +67,22 @@ describe('BasicTracerProvider - Node', () => {
const generalLimits = tracer.getGeneralLimits();
assert.strictEqual(generalLimits.attributeValueLengthLimit, 125);
assert.strictEqual(spanLimits.attributeValueLengthLimit, 109);
delete process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT;
delete process.env.OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT;
});
});

describe('when attribute count limit is defined via env', () => {
afterEach(function () {
delete process.env.OTEL_ATTRIBUTE_COUNT_LIMIT;
delete process.env.OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT;
});

it('should general attribute count limit as defined with env', () => {
process.env.OTEL_ATTRIBUTE_COUNT_LIMIT = '25';
const tracer = new BasicTracerProvider({}).getTracer(
'default'
) as Tracer;
const generalLimits = tracer.getGeneralLimits();
assert.strictEqual(generalLimits.attributeCountLimit, 25);
delete process.env.OTEL_ATTRIBUTE_COUNT_LIMIT;
});
it('should have span attribute count limit value same as general limit value', () => {
process.env.OTEL_ATTRIBUTE_COUNT_LIMIT = '20';
Expand All @@ -88,7 +93,6 @@ describe('BasicTracerProvider - Node', () => {
const spanLimits = tracer.getSpanLimits();
assert.strictEqual(generalLimits.attributeCountLimit, 20);
assert.strictEqual(spanLimits.attributeCountLimit, 20);
delete process.env.OTEL_ATTRIBUTE_COUNT_LIMIT;
});
it('should have span and general attribute count limits as defined in env', () => {
process.env.OTEL_ATTRIBUTE_COUNT_LIMIT = '20';
Expand All @@ -100,8 +104,6 @@ describe('BasicTracerProvider - Node', () => {
const generalLimits = tracer.getGeneralLimits();
assert.strictEqual(generalLimits.attributeCountLimit, 20);
assert.strictEqual(spanLimits.attributeCountLimit, 35);
delete process.env.OTEL_ATTRIBUTE_COUNT_LIMIT;
delete process.env.OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT;
});
});
});
Expand Down