Skip to content

Conversation

@sakina1303
Copy link

@sakina1303 sakina1303 commented Oct 30, 2025

Fix: Allow mock.property() to work with process.env variables

Description

This PR fixes a TypeError that occurs when using mock.property() from the node:test module on process.env properties.

Error before:
TypeError: 'process.env' does not accept an accessor(getter/setter) descriptor

Fix:
When the target is process.env, mock.property() now performs a direct value assignment instead of using an accessor descriptor.
This ensures environment variables can be safely mocked and restored without throwing errors.

Reproduction Steps

import { mock } from "node:test";

console.log(Object.getOwnPropertyDescriptor(process.env, "PATH"));
mock.property(process.env, "PATH", "foo");
console.log(process.env.PATH);

With this fix:
{ value: '/usr/local/bin:...', writable: true, enumerable: true, configurable: true }
foo

Tests Added:
Added a new test file:
test/parallel/test-env-mock-process-env.js

The test validates:

  • The mock operation on process.env variables works correctly.
  • The environment variable can be restored safely.

Example output:
Mocked process.env.TEST_ENV successfully
Restored original process.env.TEST_ENV

Checklist:

  • [✅] Includes test coverage for the change.
  • [✅] make -j4 test passes locally.
  • [✅] No documentation changes required.
  • [✅] Commit message follows Node.js guidelines.

Developer’s Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license indicated in the file; or
(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license; or
(c) The contribution was provided directly to me by another person who certified (a), (b), or (c); and
(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including my sign-off) is maintained indefinitely.

fixes #60486

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Oct 30, 2025
@regseb
Copy link
Contributor

regseb commented Oct 30, 2025

The mock.property() method must return a ProxyConstructor, with the new condition it will return process.env (which isn't a ProxyConstructor).

The lib/test.js file no longer exports test functionality (test, mock...), and its content is identical to the new test/parallel/test-env-mock-process-env.js file (except comment line 24). This new file doesn't use the mock.property() method, so it cannot test the change made in that method.

🤨

@sakina1303
Copy link
Author

Thanks for catching that, @regseb! I’ve updated the implementation so that the process.env case now correctly returns a ProxyConstructor instead of the environment object. Also cleaned up the test file to properly verify both the mocking and restoring behavior using mock.property(). Please have a look at the latest changes and let me know if any further improvements are needed.

validateObject(object, 'object');
validateStringOrSymbol(propertyName, 'propertyName');

if (object === process.env) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems very specific to process.env. What does it make this object special that prevents mocking? Can we have a generic system?

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.58%. Comparing base (8ea8d4c) to head (e322698).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/mock/mock.js 18.18% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60499      +/-   ##
==========================================
+ Coverage   88.57%   88.58%   +0.01%     
==========================================
  Files         704      704              
  Lines      207828   207868      +40     
  Branches    40040    40052      +12     
==========================================
+ Hits       184079   184138      +59     
+ Misses      15802    15766      -36     
- Partials     7947     7964      +17     
Files with missing lines Coverage Δ
lib/internal/test_runner/mock/mock.js 97.80% <18.18%> (-0.93%) ⬇️

... and 32 files with indirect coverage changes

🚀 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mock.property(): 'process.env' does not accept an accessor(getter/setter) descriptor

4 participants