- 
                Notifications
    You must be signed in to change notification settings 
- Fork 947
          test(sdk-node): use process.env consistently in tests
          #6052
        
          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
Conversation
| Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@           Coverage Diff           @@
##             main    #6052   +/-   ##
=======================================
  Coverage   95.16%   95.16%           
=======================================
  Files         316      316           
  Lines        9198     9207    +9     
  Branches     2057     2075   +18     
=======================================
+ Hits         8753     8762    +9     
  Misses        445      445           🚀 New features to boost your workflow:
 | 
| import * as Sinon from 'sinon'; | ||
| import { NodeSDK } from '../src'; | ||
| import { env } from 'process'; | ||
| import * as process from 'process'; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process is already in the global scope https://nodejs.org/api/globals.html#process so I think it is safe to remove this line. Also I think this import opens a door to load a package if installed (https://www.npmjs.com/package/process)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I wasn't sure why it was explicitly imported, so I left it there. I have removed it now. FWIW, process is imported in other places in the codebase.
Also I think this import opens a door to load a package if installed
The preferred way to guard against this is to import Node core modules with the node: prefix (for example, node:fs instead of fs). Newer core modules actually requires the prefix (for example, node:sqlite would work, but sqlite would not).
Which problem is this PR solving?
sdk.test.tsinsdk-nodeappears to be the only place in the codebase that uses a destructuredprocess.env. It even does so inconsistently within the file.Fixes # N/A
Short description of the changes
This commit updates the test file to use
process.envconsistently.Type of change
None of the items in this list really apply.
How Has This Been Tested?
Checklist: