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
chore: re-enable and fix some BrowserWindow tests on Linux #32575
base: main
Are you sure you want to change the base?
chore: re-enable and fix some BrowserWindow tests on Linux #32575
Conversation
spec-main/api-browser-window-spec.ts
Outdated
@@ -1120,12 +1122,12 @@ describe('BrowserWindow module', () => { | |||
transparent: true | |||
}); | |||
|
|||
const maximize = emittedOnce(w, 'resize'); | |||
const maximize = emittedOnce(w, 'maximize'); |
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.
@mlaurencin, can you confirm this change looks OK? These lines got added in #26586, but I'm assuming there was just a copy-and-paste typo since the variable name and event name mismatched. Seems like they mostly worked since resize
events do fire on maximize and unmaximize, but on Linux this wasn't passing since there's a bit of delay between calling maximize()
and it actually maximizing, so it needs to wait on the maximize
event, the resize
event will fire too early.
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.
No, these were not copy-and-paste typos. Maximizing transparent windows on Windows is done by resizing the window rather than actually maximizing it. If you change it back, I believe it should pass the Windows test it is currently timing out on. If it is failing on Linux with the previous implementation, then there may just need to be a test for each system.
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.
Thanks for the context! If it's Windows-specific to transparent windows, then yea, perhaps this version should be a Windows-only test and the others can test with the regular maximize
events, or simply a ternary there in the emittedOnce
call which selects the event based on platform.
4b20b81
to
56dc508
Compare
🤔 Events are working again on Linux locally, but not in CI. |
56dc508
to
2c6ad65
Compare
2c6ad65
to
78fcb2c
Compare
78fcb2c
to
5ad8465
Compare
Description of Change
Now that #32441 has landed, re-enable some tests on Linux which were disabled in 27599a8 due to events not firing, enable a few others, and fix tests.
Checklist
npm test
passesRelease Notes
Notes: none