fix: add tests for update-notifier and adjust code #301
Conversation
test/updater.test.js
Outdated
| test('verify updater', function (t) { | ||
| t.plan(1); | ||
| var pkg = { | ||
| "name": "snyk", |
test/updater.test.js
Outdated
| resolveStub.onSecondCall().returns('falseFile'); | ||
|
|
||
| t.equal(updateCheck(), false, 'Notifier was not started on missing package.json'); | ||
| resolveStub.restore(); |
There was a problem hiding this comment.
All the .restore() stuff should be in t.teardown callback. Because if test fails unrestored stub can affect other tests.
test/updater.test.js
Outdated
| const updateNotifier = require('update-notifier'); | ||
|
|
||
| // Fake location of the package.json file and verify the code behaves well | ||
| test('missing package.json', function (t) { |
src/lib/updater.ts
Outdated
| import * as p from 'path'; | ||
|
|
||
| export function updateCheck() { | ||
| const root = p.resolve(__dirname, p.normalize('../..')); |
There was a problem hiding this comment.
You don't need p.normalize here - it is not user input (already normalised).
There was a problem hiding this comment.
I believe we can do something like const pkgPath = p.join(root, '../../package.json'); instead of this.
There was a problem hiding this comment.
tests also run on Windows env, so I do need p.normalize. And this file is not a test, it runs in CLI.
There was a problem hiding this comment.
Just check it path.win32.join('bla1', 'bla2', 'bla3', 'Bla4', '../package.json'); I believe join already doing normalize inside.
test/updater.test.js
Outdated
| const updateNotifier = require('update-notifier'); | ||
|
|
||
| // Fake location of the package.json file and verify the code behaves well | ||
| test('missing package.json', function (t) { |
There was a problem hiding this comment.
Can we add test which actually checks that notifier works (I mean produces some output)?
There was a problem hiding this comment.
I can't run notifier during the test as it spawns a child process and probably during testing this is not possible. Plus, I do not want to test external library. All I want is to make sure I'm able to init it successfully with the API they claimed. This way, if API changes I'll catch that during tests.
There was a problem hiding this comment.
But in the test "verify updater" you're exactly testing external library? At least maybe better to mock fs.readFileSync?
|
|
||
| t.equal(updateCheck(), false, 'Notifier was not started on missing package.json'); | ||
| resolveStub.restore(); | ||
| t.end(); |
There was a problem hiding this comment.
I'm not sure you need t.plan(x); and t.end(); in this tests.
There was a problem hiding this comment.
tests fail without that. I can remove t.plan, but without t.end it fails as it thinks the test is unfinished
There was a problem hiding this comment.
Let's remove t.plan(x); at least.
yuliabaron
force-pushed
the
fix/test_updater
branch
3 times, most recently
from
f8daae5
to
04a6b6d
Compare
yuliabaron
force-pushed
the
fix/test_updater
branch
from
04a6b6d
to
d2cfcb0
Compare
|
🎉 This PR is included in version 1.116.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What does this PR do?
Adds tests for update-notifier API