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.

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dsanders11
Copy link
Member

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

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added

Release Notes

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 21, 2022
@@ -1120,12 +1122,12 @@ describe('BrowserWindow module', () => {
transparent: true
});

const maximize = emittedOnce(w, 'resize');
const maximize = emittedOnce(w, 'maximize');
Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

@dsanders11 dsanders11 force-pushed the enable-browserwindow-tests-linux branch from 4b20b81 to 56dc508 Compare January 21, 2022 03:09
@dsanders11 dsanders11 marked this pull request as draft January 21, 2022 04:14
@dsanders11
Copy link
Member Author

🤔 Events are working again on Linux locally, but not in CI.

@dsanders11 dsanders11 force-pushed the enable-browserwindow-tests-linux branch from 78fcb2c to 5ad8465 Compare June 25, 2022 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants