-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few small notes
test/bulk-update/bulk-update.test.js
Outdated
|
||
describe('BulkUpdater', () => { | ||
describe('loadFromUrl', () => { | ||
it('should load data from a URL'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not testing anything here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated these tests with dependency injection for fetch
test/bulk-update/bulk-update.test.js
Outdated
it('should call migration.init'); | ||
|
||
it('should call migration.migrate'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a new test for this after some refactoring.
@@ -71,7 +71,7 @@ describe('DocumentManager', () => { | |||
|
|||
it('loads a mock file'); | |||
|
|||
it('loads a draft file', async () => { | |||
it.skip('loads a draft file', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the skip? If it needs more work, do we have a ticket for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Occasionally this test would time out, but I updated these tests with dependency injection for fetch so no more skip needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good!
Resolves: MWPW-141440