US152268 - Migrate dialog-confirm and dialog-ifrau vdiff tests #3935
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
| @@ -18,24 +18,13 @@ | |||
| <body class="d2l-typography"> | |||
| <d2l-button id="open">Open Dialog</d2l-button> | |||
There was a problem hiding this comment.
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 🎉
| <script> | ||
| const dialog = document.getElementById('ifrau-dialog'); | ||
| dialog.addEventListener('d2l-dialog-open', () => { | ||
| window.parent.postMessage('ifrau-dialog-open', '*'); |
There was a problem hiding this comment.
Are these postMessage values no longer being used?
There was a problem hiding this comment.
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?
| `; | ||
|
|
||
| const confirmDialog = html` | ||
| <d2l-dialog-confirm id="confirm" title-text="Title" text="Are you sure?" opened> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If we do, I'm thinking those unit tests should be for the general mixin case.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ok, merged the change in @brightspace-ui/testing and now CI here is passing.
There was a problem hiding this comment.
Yep, exactly. Looks great!
Co-authored-by: github-actions <[email protected]>
|
🎉 This PR is included in version 2.141.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
Like the previously migrated
dialogtests, I've turned off thenativetests for now.FYI @grant-cleary since you know more about the
ifrautests.Final report: https://vdiff.d2l.dev/BrightspaceUI/core/5d76917d6b2e6eeec4a851c6bbbabe9f8e975f5a/2023-08-28_19-00-54/report/