-
Notifications
You must be signed in to change notification settings - Fork 15k
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
Refactor <webview> tag tests #12886
Refactor <webview> tag tests #12886
Conversation
4f4136f
to
7fdc029
Compare
7fdc029
to
21a2d84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but before I Approve -- If I'm reading correctly, this is not about a functional change, but about making the log output more readable. Is that correct?
@ckerr In several case it does make output more readable. And that was initial reason for some refactoring. But mostly the changes here are about making code simpler, and easier to read and change. (Tests have to be super simple.) |
@ckerr Did I answer you question? Anything else? |
A good example would be a test with two event listeners: it('loads native modules when navigation happens', (done) => {
<...>
const listener = () => {
webview.removeEventListener('did-finish-load', listener)
const listener2 = (e) => {
assert.equal(e.message, 'function')
done()
}
webview.addEventListener('console-message', listener2)
webview.reload()
}
webview.addEventListener('did-finish-load', listener)
webview.setAttribute('nodeintegration', 'on')
webview.src = `file://${fixtures}/pages/native-module.html`
document.body.appendChild(webview)
}) changed to it('loads native modules when navigation happens', async function () {
<...>
await loadWebView(webview, {
nodeintegration: 'on',
src: `file://${fixtures}/pages/native-module.html`
})
webview.reload()
const {message} = await waitForEvent(webview, 'console-message')
assert.equal(message, 'function')
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that answers my question, I was just making sure that I wasn't misunderstanding the scope of the PR.
As for the PR itself, these changed tests are a lot more straightforward / readable. Thanks for these improvements!
Rewrite tests with
async/await
.