Various unit test fixes for unit:security#115
Conversation
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Awesome, thank you for the help with this!
| "test:unit:apitests": "node ./dist/bin/harper.js stop && mocha 'unitTests/apiTests/**/*-test.mjs' --config unitTests/.mocharc.json --enable-source-maps", | ||
| "test:unit:logging": "mocha 'unitTests/utility/logging/*.js' --config unitTests/.mocharc.json --enable-source-maps", | ||
| "test:unit:security": "mocha 'unitTests/security/**/*.js' --config unitTests/.mocharc.json --enable-source-maps", | ||
| "test:unit:security": "node --expose-internals node_modules/.bin/_mocha 'unitTests/security/**/*.js' --config unitTests/.mocharc.json --enable-source-maps", |
There was a problem hiding this comment.
I needed to add this so the jsLoader error checking unit tests would pass. Without it the error stacks don’t include filenames. I’m not entirely sure why this works, but it’s used the same way in harperdb.
There was a problem hiding this comment.
This is an artifact from the PR that added this feature in harperdb : https://github.com/HarperFast/harperdb/pull/2515
There was a problem hiding this comment.
I got curious about this and found that the additions to unitTests/.mocharc.json in 10fb9f9 preserve the ability to run these with just npm run test:unit unitTests/security. The expose-internals option doesn't seem to negatively impact the other tests and excluding the jsLoader/fixture modules make mocha's recursive test search much happier (because some of those are intentionally invalid).
There was a problem hiding this comment.
What about using node node_modules/.bin/_mocha instead of mocha? That's not necessary, right? (based on Ethan's commit, looks like we have another, cleaner way to pass in --expose-internals)
Obviously I've already approved this, just curious :).
There was a problem hiding this comment.
What about using
node node_modules/.bin/_mochainstead ofmocha? That's not necessary, right? (based on Ethan's commit, looks like we have another, cleaner way to pass in--expose-internals) Obviously I've already approved this, just curious :).
It does not seem to be necessary with my changes to .mocharc.json.
There was a problem hiding this comment.
I'm planning to do a broader cleanup of those test scripts in package.json in the main-branch-targeting PR I'll put together once this PR is merged. So IMO this is good to go as-is.
4 participants
Security unit test fixes
Fixes: #77