Viewer: Add tests for fileuploader #1184
Conversation
|
node 4 and 5 can't do default params, even with the |
|
What do you think I should do here? We'll have to test against the browserified built files, but that doesn't feel very testable. |
|
I guess we could also update logger.js to remove default args. That's a shame. |
|
@ebidel you could do it the old way with if statements or just run it against compiled source. Only downsize is that it takes longer because you need compilation and error messages will be a bit more cryptic. |
|
PTAL. Travis is 😄 again. |
|
yay tests! but man you and your yarn diffs haha :) |
| testHelpers.setupGlobals(); | ||
| }); | ||
|
|
||
| it('constructor', () => { |
| }); | ||
|
|
||
| it('document responds to drag and drop events', () => { | ||
| // TODO: test drop event on document. Callback is not getting called |
There was a problem hiding this comment.
you can do it.skip() for this so the test can be written and we can easily turn it on if that ever gets fixed. Will probably need to be separate from below assertions for that, though
|
|
||
| const PAGE = fs.readFileSync(path.join(__dirname, '../app/index.html'), 'utf8'); | ||
|
|
||
| function setupGlobals() { |
There was a problem hiding this comment.
is there a way we can better isolate this global setup from other tests? Seems like mocha should have some kind of facility for this
|
PTAL |
| it.skip('document responds to drag and drop events', () => { | ||
| new FileUploader(_ => { | ||
| assert.ok(true, 'file change callback is called after drop event'); | ||
| done(); |
There was a problem hiding this comment.
nit: no done callback here
| const logger = require('../app/src/logger'); | ||
|
|
||
| function assertUIReset(uploader) { | ||
| assert.ok(!logger.el.classList.contains('show')); |
There was a problem hiding this comment.
how does this get a reference to the logger instance?
There was a problem hiding this comment.
The logger module creates a singleton to a DOM node and FileUploader uses logger internally to the module. It's a bit crude, but the intent here is a test that checks the state of the DOM. We don't really have another way to check that UI is working as it should.
There was a problem hiding this comment.
doesn't that mean logger retains an element from the original jsdom document created, not a new one each time?
There was a problem hiding this comment.
That's fine. It's a singleton. Anyways we're not going to change the impl of logger with a PR that adds tests. Let's be happy there are tests coming in :)
|
|
||
| // call before other src imports so DOM code that relies on `document` and | ||
| // `window` have them defined. | ||
| setupGlobals(); |
There was a problem hiding this comment.
can we do this without side effects? Caller just needs to call helper.setupGlobals() before requiring anything that assumes window and document?
There was a problem hiding this comment.
It's nice not to have to. I'm going to write 6 more test files and they'll have have to do that.
There was a problem hiding this comment.
Side effects from require should be avoided whenever possible, so I'd say definitely delete or write a @fileoverview explaining that requiring the file will set those up so it's not a surprise to others writing tests.
Deleting shouldn't require any other changes, though, since you already testHelpers.setupGlobals(); in beforeEach anyways, right? (and all the other test files will have to do the same)
|
PTAL |
|
@patrickhulce if you're on the clock today, can you PTAL? |
|
Ping. Let's land these tests so I can write some more! |
|
|
||
| // call before other src imports so DOM code that relies on `document` and | ||
| // `window` have them defined. | ||
| setupGlobals(); |
There was a problem hiding this comment.
Side effects from require should be avoided whenever possible, so I'd say definitely delete or write a @fileoverview explaining that requiring the file will set those up so it's not a surprise to others writing tests.
Deleting shouldn't require any other changes, though, since you already testHelpers.setupGlobals(); in beforeEach anyways, right? (and all the other test files will have to do the same)
|
|
||
| const PAGE = fs.readFileSync(path.join(__dirname, '../app/index.html'), 'utf8'); | ||
|
|
||
| function setupGlobals() { |
There was a problem hiding this comment.
total nit, but maybe a different name than setupGlobals? setupViewJsDom or something terrible like that would be more clear about what it's doing.
|
PTAL |
R: @brendankenny @patrickhulce
Adds tests for viewer's
FileUploaderclass. Part of #1108.