Skip to content

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 10, 2025

Which problem is this PR solving?

The ConfigProvider tests were deleting a number of environment variables after every test, and this logic was duplicated in a few places. This is a bit error prone.

Fixes # N/A

Short description of the changes

The tests seem to pass with a clean process.env provided to each test, and Mocha's afterEach() hook is inherited, so the logic can be simplified a good bit while maintaining semantics.

Type of change

  • Bug fix (non-breaking change which fixes an issue) Not really a bug, but more of a simplification.

How Has This Been Tested?

  • Existing test suite

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@cjihrig cjihrig requested a review from a team as a code owner October 10, 2025 19:36
@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.22%. Comparing base (a26422d) to head (861fe22).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6004   +/-   ##
=======================================
  Coverage   95.22%   95.22%           
=======================================
  Files         311      311           
  Lines        8603     8603           
  Branches     1801     1801           
=======================================
  Hits         8192     8192           
  Misses        411      411           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement, it's a great idea so we don't forget any variables 😄

I added a suggestion, because we might evolve those tests and need some pre determined variables. This way you save the original values (doesn't need to be inside a before), and restore after every test

Comment on lines 356 to 362
before(function () {
process.env = {};
});

afterEach(function () {
process.env = {};
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
before(function () {
process.env = {};
});
afterEach(function () {
process.env = {};
});
const _origEnvVariables = process.env
afterEach(function () {
process.env = _origEnvVariables;
});

… variables

The ConfigProvider tests were deleting a number of environment
variables after every test, and this logic was duplicated in a
few places. This is a bit error prone.

The tests seem to pass with a clean process.env provided to each
test, and Mocha's afterEach() hook is inherited, so the logic can
be simplified a good bit while maintaining semantics.
@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 11, 2025

I've made the suggested changes, with a few notes:

  1. I had to make copies of process.env, otherwise _origEnvVariables would just be the same object.
  2. If the user already has some of these environment variables in their environment, they could potentially interfere with the tests, but it sounds like that is fine.
  3. I also fixed up the xxxx placeholders to reference actual pull request numbers.

@maryliag maryliag added this pull request to the merge queue Oct 11, 2025
Merged via the queue into open-telemetry:main with commit fb3aeef Oct 11, 2025
25 checks passed
@otelbot
Copy link
Contributor

otelbot bot commented Oct 11, 2025

Thank you for your contribution @cjihrig! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

@cjihrig cjihrig deleted the config-tests branch October 11, 2025 13:24
cjihrig added a commit to cjihrig/opentelemetry-js that referenced this pull request Oct 13, 2025
PR open-telemetry#6004 simplified the environment restoration logic after each
test. However, process.env is a special object that converts all
of its values to strings automatically. That behavior was lost
in open-telemetry#6004, which could potentially lead to surprising behavior.
This commit changes the process.env restoration logic to maintain
the original special object.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants