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.

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

tests: improve drag-and-drop coverage #9314

Merged
merged 6 commits into from
Jul 8, 2019
72 changes: 51 additions & 21 deletions lighthouse-viewer/test/drag-and-drop-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@

/* eslint-env jest */

const assert = require('assert');

const testHelpers = require('./test-helpers.js');

// Called before other src import so code that relies on `document` and
Expand All @@ -17,8 +15,12 @@ testHelpers.setupJsDomGlobals();

const DragAndDrop = require('../app/src/drag-and-drop.js');

function assertUIReset() {
assert.ok(!document.querySelector('.drop_zone').classList.contains('dropping'));
function createCustomEvent(type, key, value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: WDYT about the caller passing {key: value} to make it a little clearer below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean add a comment here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's ignore this comment in favor of #9314 (comment)

const customDropEvent = new window.CustomEvent(type);
if (key && value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now it's a bit odd that the callers do createCustomEvent('drag', 'dataTransfer', null) when it seems like that doesn't really do anything, is this intentional?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing this fn entirely would be better.

// create custom drop event with mock files in dataTransfer
const event = new CustomEvent('drop');
event.dataTransfer = {
  files: ['mock file'],
};
document.dispatchEvent(event);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah ignore what I was saying, let's go with @connorjclark !

customDropEvent[key] = value;
}
return customDropEvent;
}

describe('DragAndDrop', () => {
Expand All @@ -29,31 +31,59 @@ describe('DragAndDrop', () => {

afterEach(testHelpers.cleanupJsDomGlobals);

// TODO: test drop event on document. Callback is not getting called
// because jsdom doesn't support clipboard API: https://github.com/tmpvar/jsdom/issues/1568/.
it.skip('document responds to drag and drop events', done => {
const callback = _ => {
assert.ok(true, 'file change callback is called after drop event');
done();
};
it('document responds to drop event with file', () => {
const mockCallback = jest.fn();
new DragAndDrop(mockCallback);

// create custom drop event with mock files in dataTransfer
document.dispatchEvent(createCustomEvent('drop', 'dataTransfer', {
files: ['mock file'],
}));
expect(mockCallback).toBeCalledWith('mock file');
});

it('document responds to drop event without file', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('document responds to drop event without file', () => {
it('document ignores drop event without file', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix it. thanks for suggestion.

const mockCallback = jest.fn();
new DragAndDrop(mockCallback);

document.dispatchEvent(createCustomEvent('drop', 'dataTransfer', null));
expect(mockCallback).not.toBeCalled();
});

it('document responds to dragover event with file', () => {
const mockCallback = jest.fn();
new DragAndDrop(mockCallback);

const dragoverEvent = createCustomEvent('dragover', 'dataTransfer', {
files: ['mock file'],
});
document.dispatchEvent(dragoverEvent);
expect(dragoverEvent.dataTransfer.dropEffect).toEqual('copy');
});

new DragAndDrop(callback);
it('document responds to dragover event without file', () => {
const mockCallback = jest.fn();
new DragAndDrop(mockCallback);

document.dispatchEvent(new window.CustomEvent('drop'));
const dragoverEvent = createCustomEvent('dragover', 'dataTransfer', null);
document.dispatchEvent(dragoverEvent);
expect(dragoverEvent.dataTransfer).toBeUndefined();
});

it('document responds to drag and drop events', () => {
// eslint-disable-next-line no-unused-vars
const dragAndDrop = new DragAndDrop();
it('document responds to mouseleave event when not dragging', () => {
new DragAndDrop(jest.fn);

document.dispatchEvent(new window.CustomEvent('mouseleave'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we align on createCustomEvent while we're at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, I will made the change align it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow up with #9314 (comment)

assertUIReset();
expect(document.querySelector('.drop_zone').classList.contains('dropping')).toBeFalsy();
});

it('document responds to mouseleave and dragenter events', () => {
new DragAndDrop(jest.fn);

document.dispatchEvent(new window.CustomEvent('dragenter'));
assert.ok(document.querySelector('.drop_zone').classList.contains('dropping'));
expect(document.querySelector('.drop_zone').classList.contains('dropping')).toBeTruthy();

// TODO: see note above about drop event testing.
// document.dispatchEvent(new window.CustomEvent('drop'));
// assertUIReset();
document.dispatchEvent(new window.CustomEvent('mouseleave'));
expect(document.querySelector('.drop_zone').classList.contains('dropping')).toBeFalsy();
});
});