Skip to content

Commit

Permalink
Fix flakey ProfileModal tests
Browse files Browse the repository at this point in the history
The ProfileModal tests would sometimes fail with this error:

```
FAILED TESTS:
  ✖ "after each" hook for "shows modal on "openProfile" event"
    Chrome Headless 129.0.0.0 (Linux x86_64)
  Error: Failed to execute 'showModal' on 'HTMLDialogElement': The element is not in a Document.
```

The `HTMLDialogElement.showModal` call happens in an effect when the
`ModalDialog` component is rendered with the `isClosed` prop set to false. In
the ProfileModal tests, the component was rendered in a disconnected DOM node,
so this error should have happened on every run. However the
`emitter.publish("openProfile")` call which triggered this render was not
wrapped in `act` and so the effect which calls `showModal` was scheduled, but
often did not actually run before the component was unmounted in the `afterEach`
hook.

Fix the issue by:

 - Wrapping all `emitter.publish("openProfile")` calls in `act`, so they
   synchronously execute the effect.
 - Rendering the `ProfileModal` component in a connected DOM container
   which is removed after the test runs
 - For consistency, update the `NotebookModal` tests to work in the same
   way as the ProfileModal tests, with a single container element which
   is removed at the end of the test
  • Loading branch information
robertknight committed Oct 14, 2024
1 parent 25b6147 commit 0340a9f
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 15 deletions.
32 changes: 19 additions & 13 deletions src/annotator/components/test/NotebookModal-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,28 @@ import NotebookModal, { $imports } from '../NotebookModal';
describe('NotebookModal', () => {
const notebookURL = 'https://test.hypothes.is/notebook';

let container;
let components;
let eventBus;
let emitter;

const outerSelector = 'dialog[data-testid="notebook-outer"]';

const createComponent = config => {
const attachTo = document.createElement('div');
document.body.appendChild(attachTo);

const component = mount(
<NotebookModal
eventBus={eventBus}
config={{ notebookAppUrl: notebookURL, ...config }}
/>,
{ attachTo },
{ attachTo: container },
);
components.push([component, attachTo]);
components.push(component);
return component;
};

beforeEach(() => {
container = document.createElement('div');
document.body.append(container);
components = [];
eventBus = new EventBus();
emitter = eventBus.createEmitter();
Expand All @@ -46,10 +46,8 @@ describe('NotebookModal', () => {
});

afterEach(() => {
components.forEach(([component, container]) => {
component.unmount();
container.remove();
});
components.forEach(component => component.unmount());
container.remove();
$imports.$restore();
});

Expand All @@ -70,7 +68,9 @@ describe('NotebookModal', () => {
assert.isFalse(outer.exists());
assert.isFalse(wrapper.find('iframe').exists());

emitter.publish('openNotebook', 'myGroup');
act(() => {
emitter.publish('openNotebook', 'myGroup');
});
wrapper.update();

outer = wrapper.find(outerSelector);
Expand All @@ -86,7 +86,9 @@ describe('NotebookModal', () => {
it('creates a new iframe element on every "openNotebook" event', () => {
const wrapper = createComponent();

emitter.publish('openNotebook', '1');
act(() => {
emitter.publish('openNotebook', '1');
});
wrapper.update();

const iframe1 = wrapper.find('iframe');
Expand All @@ -95,7 +97,9 @@ describe('NotebookModal', () => {
addConfigFragment(notebookURL, { group: '1' }),
);

emitter.publish('openNotebook', '1');
act(() => {
emitter.publish('openNotebook', '1');
});
wrapper.update();

const iframe2 = wrapper.find('iframe');
Expand All @@ -105,7 +109,9 @@ describe('NotebookModal', () => {
);
assert.notEqual(iframe1.getDOMNode(), iframe2.getDOMNode());

emitter.publish('openNotebook', '2');
act(() => {
emitter.publish('openNotebook', '2');
});
wrapper.update();

const iframe3 = wrapper.find('iframe');
Expand Down
13 changes: 11 additions & 2 deletions src/annotator/components/test/ProfileModal-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import ProfileModal from '../ProfileModal';
describe('ProfileModal', () => {
const profileURL = 'https://test.hypothes.is/user-profile';

let container;
let components;
let eventBus;
let emitter;
Expand All @@ -19,19 +20,23 @@ describe('ProfileModal', () => {
eventBus={eventBus}
config={{ profileAppUrl: profileURL, ...config }}
/>,
{ attachTo: container },
);
components.push(component);
return component;
};

beforeEach(() => {
container = document.createElement('div');
document.body.append(container);
components = [];
eventBus = new EventBus();
emitter = eventBus.createEmitter();
});

afterEach(() => {
components.forEach(component => component.unmount());
container.remove();
});

it('does not render anything while the modal is closed', () => {
Expand All @@ -42,7 +47,9 @@ describe('ProfileModal', () => {
it('shows modal on "openProfile" event', () => {
const wrapper = createComponent();

emitter.publish('openProfile');
act(() => {
emitter.publish('openProfile');
});
wrapper.update();

assert.isTrue(wrapper.find(outerSelector).exists());
Expand All @@ -54,7 +61,9 @@ describe('ProfileModal', () => {
it("removes the modal's content on closing", () => {
const wrapper = createComponent();

emitter.publish('openProfile');
act(() => {
emitter.publish('openProfile');
});
wrapper.update();

assert.isTrue(wrapper.find(outerSelector).exists());
Expand Down

0 comments on commit 0340a9f

Please sign in to comment.