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.

Sign up for GitHub

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

Jump to bottom

exitCode always 0 when encountering stale approvals #125

Open
davidalpert opened this issue Oct 13, 2021 · 2 comments · May be fixed by #126
Open

exitCode always 0 when encountering stale approvals #125

davidalpert opened this issue Oct 13, 2021 · 2 comments · May be fixed by #126

Comments

@davidalpert
Copy link

davidalpert commented Oct 13, 2021 •
edited
Loading

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 6e8e859 which introduced this

Approvals.NodeJS/lib/Approvals.js

Lines 70 to 72 in 6e8e859

if(staleApprovals.length){
throw 'ERROR: Found stale approvals files: \n - ' + staleApprovals.join('\n - ') + '\n';
}

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 added a commit to davidalpert/Approvals.NodeJS that referenced this issue Oct 13, 2021
@davidalpert
approvalsGH-125 set exit code when failing postRunCleanup
8baee0b 
Closes approvals#125
@davidalpert davidalpert linked a pull request Oct 13, 2021 that will close this issue
Open
@davidalpert
Copy link
Author

hacktoberfest-ready 🎃 😁

@staxmanade
Copy link
Member

staxmanade commented Nov 22, 2021 •
edited
Loading

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

GH-125 set exit code when failing postRunCleanup davidalpert/Approvals.NodeJS
2 participants
@davidalpert @staxmanade