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.

Sign up for GitHub

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

Jump to bottom

US152268 - Migrate dialog-confirm and dialog-ifrau vdiff tests #3935

Merged
merged 5 commits into from
Aug 28, 2023
Merged

US152268 - Migrate dialog-confirm and dialog-ifrau vdiff tests #3935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b98ec8b
US152268 - Migrate dialog-confirm and dialog-ifrau vdiff tests
svanherk Aug 28, 2023
a1c73fe
Migrate existing goldens to new location
svanherk Aug 28, 2023
5d76917
Add dialog-mixin event unit tests
svanherk Aug 28, 2023
15b20a4
Updating vdiff goldens (#3936)
github-actions[bot] Aug 28, 2023
7521163
Update @brightspace-ui/testing
svanherk Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/dialog/dialog-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ export const DialogMixin = superclass => class extends RtlMixin(superclass) {
}

const dialogOuterClasses = {
'vdiff-target': true,
'd2l-dialog-outer': true,
'd2l-dialog-outer-full-height': this._autoSize && this._isFullHeight,
'd2l-dialog-outer-overflow-bottom': this._overflowBottom,
Expand Down
75 changes: 75 additions & 0 deletions components/dialog/test/dialog-confirm.vdiff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import '../../button/button.js';
import '../dialog-confirm.js';
import { expect, fixture, html } from '@brightspace-ui/testing';

const buttons = html`
<d2l-button slot="footer" primary>Yes</d2l-button>
<d2l-button slot="footer" id="cancel">No</d2l-button>
`;

const confirmDialog = html`
<d2l-dialog-confirm id="confirm" title-text="Title" text="Are you sure?" opened>
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting up by using opened attribute is definitely much cleaner, which is great. I think the helper was implicitly also testing the d2l-dialog-open & d2l-dialog-close events. I think the open event may still be covered by some unit tests (also implicitly), but I'm not sure the close event is covered. I'm wondering if it would be a good idea for us to add an explicit unit test for those events, since consumers do rely on them. Or maybe you already checked and found they are covered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'd say that's kinda covered by the dialog-mixin.vdiff.js tests. But I like the idea of adding non-vdiff, dedicated tests - I'll add some to this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do, I'm thinking those unit tests should be for the general mixin case.

Copy link
Contributor Author

@svanherk svanherk Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, just pushed a new file - it'll fail until we expose oneDefaultPreventedEvent in @brightspace-ui/testing, but is that what you had in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, merged the change in @brightspace-ui/testing and now CI here is passing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, exactly. Looks great!

${buttons}
</d2l-dialog-confirm>
`;

describe('dialog-confirm', () => {

[/*'native',*/ 'custom'].forEach((type) => {

describe(type, () => {
before(() => window.D2L.DialogMixin.preferNative = type === 'native');

[
{ screen: 'wide', viewport: { width: 800, height: 500 } },
{ screen: 'narrow', viewport: { width: 600, height: 500 } }
].forEach(({ screen, viewport }) => {
describe(screen, () => {
[
{ name: 'opened', template: confirmDialog },
{ name: 'rtl', rtl: true, template: confirmDialog },
].forEach(({ name, template, rtl }) => {
it(name, async() => {
await fixture(template, { viewport, rtl });
await expect(document).to.be.golden();
});
});
});
});

describe('internal', () => {

[
{ name: 'short', template: confirmDialog },
{ name: 'long-title', template: html`
<d2l-dialog-confirm title-text="A title that is really long and should wrap onto a second line." text="Are you sure?" opened>
${buttons}
</d2l-dialog-confirm>
` },
{ name: 'no-title', template: html`<d2l-dialog-confirm text="Are you sure?" opened>${buttons}</d2l-dialog-confirm>` },
{ name: 'long-text', template: html`
<d2l-dialog-confirm title-text="Title" text="Some confirm dialog content that should wrap onto a second line?" opened>
${buttons}
</d2l-dialog-confirm>
` },
{ name: 'long-buttons', template: html`
<d2l-dialog-confirm title-text="Title" text="Are you sure?" opened>
<d2l-button slot="footer" primary>A really long workflow button.</d2l-button>
<d2l-button slot="footer" id="cancel">Another really long workflow button.</d2l-button>
</d2l-dialog-confirm>
` },
{ name: 'multiple-paragraphs', template: html`
<d2l-dialog-confirm title-text="Title" text="Paragraph 1&#10;Paragraph 2" opened>
${buttons}
</d2l-dialog-confirm>
` }
].forEach(({ name, template }) => {
it(name, async() => {
const elem = await fixture(template, { viewport: { width: 800, height: 500 } });
await expect(elem).to.be.golden();
});
});
});
});
});
});
57 changes: 0 additions & 57 deletions components/dialog/test/dialog-confirm.visual-diff.html
View file
Open in desktop

This file was deleted.

92 changes: 0 additions & 92 deletions components/dialog/test/dialog-confirm.visual-diff.js
View file
Open in desktop

This file was deleted.

39 changes: 0 additions & 39 deletions components/dialog/test/dialog-helper.js
View file
Open in desktop

This file was deleted.

13 changes: 1 addition & 12 deletions ...st/dialog-ifrau-contents.visual-diff.html → ...log/test/dialog-ifrau-contents.vdiff.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,13 @@
<body class="d2l-typography">
<d2l-button id="open">Open Dialog</d2l-button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this d2l-button here since this test requires a whole html file anyways, but I can remove it if we want to be consistent with the look of the other tests. Left in, the dialog-ifrau tests result in no diffs 🎉


<d2l-dialog id="ifrau-dialog" title-text="Title" width="400">
<d2l-dialog id="ifrau-dialog" title-text="Title" width="400" opened>
<div>Line 1</div>
<div>Line 2</div>
<div>Line 3</div>
<div>Line 4</div>
<d2l-button slot="footer" primary>Yes</d2l-button>
<d2l-button slot="footer" id="cancel">No</d2l-button>
</d2l-dialog>

<script>
const dialog = document.getElementById('ifrau-dialog');
dialog.addEventListener('d2l-dialog-open', () => {
window.parent.postMessage('ifrau-dialog-open', '*');
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these postMessage values no longer being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, they were used to open the dialog (and then close it in reset for the next test). Now that the tests are isolated I can just start with the dialog opened. But if we want to test this functionality, I could add it back, or maybe just a dedicated test for opening and one for closing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Makes sense.

});
dialog.addEventListener('d2l-dialog-close', () => {
window.parent.postMessage('ifrau-dialog-close', '*');
});
</script>

</body>
</html>
56 changes: 56 additions & 0 deletions components/dialog/test/dialog-ifrau.vdiff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { expect, fixture, html } from '@brightspace-ui/testing';
import { Host } from 'ifrau/host.js';

describe('dialog-ifrau', () => {

[/*'native',*/ 'custom'].forEach((type) => {

describe(type, () => {
let iframeSrc;

before(() => {
const preferNative = (type === 'native' ? '' : '?preferNative=false');
iframeSrc = `${window.location.origin}/components/dialog/test/dialog-ifrau-contents.vdiff.html${preferNative}`;

const iframeOrigin = new URL(iframeSrc).origin;
const verifyPostMessage = (evt, expectedData) => {
return evt && evt.data === expectedData && iframeOrigin === evt.origin;
};
const handleIsFramedRequest = evt => {
if (!verifyPostMessage(evt, 'isFramedRequest')) return;
evt.source.postMessage({ isFramed: true }, iframeSrc);
};
window.addEventListener('message', handleIsFramedRequest, false);
});

[
{ name: 'ifrau top height lt dialog top margin', ifrau: { availableHeight: 500, top: 50 } },
{ name: 'ifrau top height gt dialog top margin', ifrau: { availableHeight: 500, top: 120 } },
{ name: 'ifrau available height lt host height', ifrau: { availableHeight: 300, top: 50 } },
{ name: 'host scrolled down', ifrau: { availableHeight: 400, top: -50 } }
].forEach(({ name, ifrau }) => {
it(name, async() => {
const elem = await fixture(html`<div id="ifrau-dialog-container" style="width: 650px; line-height: 0;"></div>`, { viewport: { width: 726, height: 300 } });
const ifrauHost = new Host(
() => elem,
iframeSrc,
{ height: '450px' }
);

await ifrauHost.registerService('dialogWC', '0.1', {
showBackdrop: () => {
return {
availableHeight: ifrau.availableHeight,
top: ifrau.top
};
},
hideBackdrop: async() => {}
});
await ifrauHost.connect();

await expect(document).to.be.golden();
}).timeout(5000); // ifrau connecting can be very slow
});
});
});
});
Loading