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

test: handle process.env in config_test.ts #2294

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 5, 2025

The tests in #2288 can fail depending on the environment variables on the system. This commit takes the environment out of the equation.

Refs: #2288

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 5, 2025
@cjihrig cjihrig requested review from Copilot and removed request for davidgamero and brendandburns March 5, 2025 14:22
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 5, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR addresses issues with tests failing due to unexpected environment variables by isolating environment values during test execution. Key changes include storing the original environment variables, replacing process.env with an empty object for testing, and restoring process.env after tests complete.

Reviewed Changes

File Description
src/config_test.ts Updated test hooks to shield tests from external process.env values

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

before(() => {
// The code being tested here references process.env and can fail
// if run on a machine with certain environment variable settings.
originalEnv = process.env;
Copy link
Preview

Copilot AI Mar 5, 2025

Choose a reason for hiding this comment

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

Assigning process.env directly to originalEnv stores a reference instead of a snapshot, which may lead to unintended side effects. Consider using a shallow copy (e.g., originalEnv = { ...process.env }) to ensure the original environment is accurately restored.

Suggested change
originalEnv = process.env;
originalEnv = { ...process.env };

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
The tests in kubernetes-client#2288 can fail depending on the environment
variables on the system. This commit takes the environment out
of the equation.

Refs: kubernetes-client#2288
@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, cjihrig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [brendandburns,cjihrig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 547ef69 into kubernetes-client:main Mar 5, 2025
8 checks passed
@cjihrig cjihrig deleted the test-fixup branch March 5, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants