Approvals.NodeJS icon indicating copy to clipboard operation
Approvals.NodeJS copied to clipboard

exitCode always 0 when encountering stale approvals

Open davidalpert opened this issue 4 years ago • 2 comments

When errorOnStaleApprovedFiles is set to true, and the code finds stale approvals, it exits in a way that leaves the process exit code at 0.

library version
node 14.5
mocha 6.2.2
approvals 3.0.5

It took me the longest time to figure out why my mocha unit tests were always returning 0 even when tests failed. Eventually I considered interaction between mocha and other libraries and it occurred to me to investigate how the approvals library may interact with exitCode.

This led me quickly to issue #7 and commit 6e8e859fd91334ee9db47dc93b1bf91f9f8d0112 which introduced this https://github.com/approvals/Approvals.NodeJS/blob/6e8e859fd91334ee9db47dc93b1bf91f9f8d0112/lib/Approvals.js#L70-L72

It turns out that mocha also uses the process.on('exit', function(..) { ... }) pattern to set process.exitCode, so when this approvals postRunCleanup handler squats on the same exit event and throws, the process seems to exit without mocha's handler properly setting the exit code.

This is significant because we expect builds to fail when tests fail. I also expect builds to fail when stale approvals are found if the flag is set such that we are running this check.

davidalpert avatar Oct 13 '21 13:10 davidalpert

hacktoberfest-ready 🎃 😁

davidalpert avatar Oct 14 '21 01:10 davidalpert

@davidalpert I think I'd like to understand more about why you are not getting the proper exit code. I ran a test locally and cannot seem to reproduce.

library version
node 14.6.0
mocha 6.0.2
approvals 3.0.5 && master/latest code
===> ./node_modules/.bin/mocha test/approvalsTests.js

  approvals
    verify
      ✓ can verify some manual text
      ✓ should verify an image
(node:81860) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
      ✓ should verify an image 2
    verifyAsJSON
      ✓ can verify some json object
      ✓ can be run with same JSON but keys ordered differently
    verifyAsJSONAndScrub
      ✓ can verify and scrub some json object


  6 passing (37ms)

/Users/jjarrett/code/personal/Approvals.NodeJS/lib/postRunCleanup.js:55
      throw new Error('ERROR: Found stale approvals files: \n  - ' + staleApprovals.join('\n  - ') + '\n');
      ^

Error: ERROR: Found stale approvals files:
  - /Users/jjarrett/code/personal/Approvals.NodeJS/test/commandlineTest.approved.txt
  - /Users/jjarrett/code/personal/Approvals.NodeJS/test/FileApprover.should_verify_two_files_match.approved.txt
  - /Users/jjarrett/code/personal/Approvals.NodeJS/test/FileApproverTests.ByteOrderMark.approved.txt
  - /Users/jjarrett/code/personal/Approvals.NodeJS/test/postRunCleanup_reporting_bad_file.approved.txt

    at module.exports (/Users/jjarrett/code/personal/Approvals.NodeJS/lib/postRunCleanup.js:55:13)
    at process.<anonymous> (/Users/jjarrett/code/personal/Approvals.NodeJS/lib/Approvals.js:42:3)
    at process.emit (events.js:326:22)
===>  echo $?
1

staxmanade avatar Nov 22 '21 17:11 staxmanade