test: add env undefined regression test #18529
Conversation
| // NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, | ||
| // DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR | ||
| // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE | ||
| // USE OR OTHER DEALINGS IN THE SOFTWARE. |
There was a problem hiding this comment.
The copyright/license header should not be included here
| // USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
||
| var exec = require('child_process').exec | ||
| var readFileSync = require('fs').readFileSync |
| // DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR | ||
| // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE | ||
| // USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
There was a problem hiding this comment.
this needs to include the require('../common'); before other requires
| var readFileSync = require('fs').readFileSync | ||
|
|
||
| process.env.test = '' | ||
| try { readFileSync('file-does-not-exist') } catch (e) {} |
There was a problem hiding this comment.
this should use either assert.fails() or common.expectsError()
|
is there a reason this is targetting v6.x rather than master? |
|
Ping @payload |
|
In the original issue (the linked comment) it was not manually tested for node 6. It failed on node 4, but not on 8. So I wanted to pin this down and thats the reason why it is targetted vor v6.x. |
|
There is a better snippet here to test a bug where empty string environment variables get treated as |
|
@nodejs/lts I guess it is your call what should be done here? |
If the test is useful on 6.x it should be useful on master as well (I think?) So the PR should be retargeted at
I think you're suggesting that this PR should be closed? |
#18463 contained a regression test that is on |
|
In that case I think this can be closed. @payload let me know if you disagree and I'll reopen. Thanks for the PR! |
|
@gibfahn I don’t think the bugfix PR itself would apply to v6.x, so it definitely makes sense to have either a separate backport of the test part, or a custom PR like this. I’d be fine with merging this |
I'd rather we do the backport of the test part for consistency. |
This test is intended to trigger a bug in process.env on Windows.
process.env may return the string 'undefined' for not existing or empty environment variables in some corner cases.
Please run this test in CI on Windows x86.
Original issue: #14593 (comment)