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

test: skip test for not supported setLoginItemSettings on mas platform #15987

Closed

Conversation

neo291
Copy link
Contributor

@neo291 neo291 commented Dec 10, 2018

Description of Change

The Browser::SetLoginItemSettings is not supported on mas and tests that are executed on this feature always fails.
To avoid this I've disabled the Browser::SetLoginItemSettings related tests on mas platform.
This is the Browser::SetLoginItemSettings function source code inside electron/atom/browser/browser_mac.mm on line 279:

void Browser::SetLoginItemSettings(LoginItemSettings settings) {
#if defined(MAS_BUILD)
  if (!platform_util::SetLoginItemEnabled(settings.open_at_login)) {
    LOG(ERROR) << "Unable to set login item enabled on sandboxed app.";
  }
#else
  if (settings.open_at_login)
    base::mac::AddToLoginItems(settings.open_as_hidden);
  else {
    RemoveFromLoginItems();
  }
#endif
}

Checklist

Release Notes

Notes: no-notes

@neo291 neo291 requested a review from a team December 10, 2018 08:08
@neo291 neo291 changed the title test: skip test for not supported get/setLoginItemSettings on mas platform test: skip test for not supported get-setLoginItemSettings on mas platform Dec 10, 2018
@neo291 neo291 changed the title test: skip test for not supported get-setLoginItemSettings on mas platform test: skip test for not supported setLoginItemSettings on mas platform Dec 10, 2018
@neo291 neo291 force-pushed the test-login-item-settings-on-mas branch 2 times, most recently from b47d131 to 018e3f9 Compare December 10, 2018 10:34
@neo291 neo291 force-pushed the test-login-item-settings-on-mas branch from 018e3f9 to 509e92e Compare December 10, 2018 13:00
@codebytere
Copy link
Member

codebytere commented Dec 10, 2018

@neo291 Browser::SetLoginItemSettings is supported on MAS. It uses a different approach than does non-sandboxed normal MacOS build. It takes longer to apply the setting changes using the Service Management framework, and so our tests flake on that sometimes, but it works.

@ckerr
Copy link
Member

ckerr commented Dec 10, 2018

@neo291 is right that this test fails a whole lot though. @codebytere any thoughts on how to make the test give a false error less often?

@neo291
Copy link
Contributor Author

neo291 commented Dec 10, 2018

@codebytere and @ckerr, thanks for the explanation, now is more clear the reason because it often fails.
In this case I think that I can proceed to close this pull request? Or you prefer to take it open to find a possible way to improve this false positive?
Let me now know if I've something to do and thanks again. 😄

@codebytere
Copy link
Member

@neo291 i'm going to close this for now but if you have some ideas about how to reduce the flakiness of this PR we'd love to chat and would welcome a PR!

@codebytere codebytere closed this Dec 22, 2018
@neo291 neo291 deleted the test-login-item-settings-on-mas branch December 23, 2018 16:45
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

3 participants