Skip to content

Bug 2044296 - Disable Sync/FxA related tests for firefox enterprise#988

Open
victhorlopez wants to merge 1 commit into
mozilla:enterprise-mainfrom
victhorlopez:bug/2044296
Open

Bug 2044296 - Disable Sync/FxA related tests for firefox enterprise#988
victhorlopez wants to merge 1 commit into
mozilla:enterprise-mainfrom
victhorlopez:bug/2044296

Conversation

@victhorlopez

@victhorlopez victhorlopez commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Description

Bugzilla: Bug-2044296

  • Skip FxA-related tests (sync, asrouter menu messages) since enterprise builds do not include Firefox Accounts UI functionality

Dependencies / Related Issues

  • Depends on:

Screenshots


Testing

  • Added tests
  • Manual testing performed

@victhorlopez victhorlopez added branch:main PR that should be merged on enterprise-main branch test-greening Fixes failing or flaky tests to restore passing CI status without changing core application logic labels Jun 2, 2026
@victhorlopez
victhorlopez marked this pull request as ready for review June 2, 2026 12:32
@victhorlopez
victhorlopez requested review from 1rneh and lissyx June 2, 2026 12:32
@victhorlopez

victhorlopez commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

I added browser_contextmenu_sendtab.js link cause it was failing and has a bunch of FxA logic

@victhorlopez
victhorlopez requested review from 1rneh and lissyx June 2, 2026 13:30

@1rneh 1rneh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added browser_contextmenu_sendtab.js link cause it was failing and has a bunch of FxA logic

Sending tabs is supported in enterprise build so I took a closer look at what was failing as we don't generally want to have regressed sending tabs to other devices.

  1. It seems only test_sendTabToDevice_showsConfirmationHint_fxa and test_sendTabToDevice_showsConfirmationHint_onOverflowMenu fail because they trigger and assert that the fxa toolbar button becomes visible.
  2. I noticed test_tab_contextmenu_send_to_mobile passed although we don't want to support Firefox Enterprise on mobile, hence we shouldn't offer that option.

In conclusion, instead of skipping the whole test suite, we can wrap those three tests if !AppConstants.MOZ_ENTERPRISE (unfortunately we can't skip-if on a task level) and we should also fix not supporting sending tabs to mobile and have test coverage for that.

@victhorlopez

Copy link
Copy Markdown
Contributor Author

I added browser_contextmenu_sendtab.js link cause it was failing and has a bunch of FxA logic

Sending tabs is supported in enterprise build so I took a closer look at what was failing as we don't generally want to have regressed sending tabs to other devices.

1. It seems only `test_sendTabToDevice_showsConfirmationHint_fxa` and `test_sendTabToDevice_showsConfirmationHint_onOverflowMenu` fail because they trigger and assert that the fxa toolbar button becomes visible.

2. I noticed `test_tab_contextmenu_send_to_mobile` passed although we don't want to support Firefox Enterprise on mobile, hence we shouldn't offer that option.

In conclusion, instead of skipping the whole test suite, we can wrap those three tests if !AppConstants.MOZ_ENTERPRISE (unfortunately we can't skip-if on a task level) and we should also fix not supporting sending tabs to mobile and have test coverage for that.

okay will do, there is this by the way for skipping on task level
#826 (comment)

https://searchfox.org/firefox-main/rev/1372522d3de2c9aeedc8020ec8d81326f1a9516e/browser/components/asrouter/tests/browser/browser_asrouter_newtab_multistage_messages.js#144

@1rneh

1rneh commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

@victhorlopez

victhorlopez commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

I added browser_contextmenu_sendtab.js link cause it was failing and has a bunch of FxA logic

Sending tabs is supported in enterprise build so I took a closer look at what was failing as we don't generally want to have regressed sending tabs to other devices.

1. It seems only `test_sendTabToDevice_showsConfirmationHint_fxa` and `test_sendTabToDevice_showsConfirmationHint_onOverflowMenu` fail because they trigger and assert that the fxa toolbar button becomes visible.

2. I noticed `test_tab_contextmenu_send_to_mobile` passed although we don't want to support Firefox Enterprise on mobile, hence we shouldn't offer that option.

In conclusion, instead of skipping the whole test suite, we can wrap those three tests if !AppConstants.MOZ_ENTERPRISE (unfortunately we can't skip-if on a task level) and we should also fix not supporting sending tabs to mobile and have test coverage for that.

1- I did the change
2- Let me know what should we do exactly here, send_Tab just checks which devices are available and if they are mobile devices only the label changes code

I would expect this never retrieves mobile devices from backend link otherwise we would need to filter them in that function for enterprise

@victhorlopez

Copy link
Copy Markdown
Contributor Author

Use this in the url ?w=1 to get rid of the indentation noise..

Comment on lines +227 to +229
availableCommands: {
"https://identity.mozilla.com/cmd/open-uri": "baz",
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this rewrote by linter?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I run ./mach lint --fix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it warrants an upstream fix as well to make sure we reduce risks of conflicts

@victhorlopez victhorlopez Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have ideas for a workaround? Should I add a helper function add_task_with_skip()

so we can have the original indentation? (and the test skips enterprise by default)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not worried about conflicts on the add_task changes at the prototype place, rather the rest of the linting changes. I assume without you get a linting failures?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume without you get a linting failures? let me try

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then we have to live with it :(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still worth I think fixing upstream to avoid risk of merge regressing just the lint?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's your opinion @1rneh

Comment on lines +236 to +238
availableCommands: {
"https://identity.mozilla.com/cmd/open-uri": "boo",
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this rewrote by linter?

@victhorlopez
victhorlopez force-pushed the bug/2044296 branch 3 times, most recently from 34d8ebb to b5bcbcd Compare June 3, 2026 13:12
@victhorlopez victhorlopez self-assigned this Jun 3, 2026
@victhorlopez
victhorlopez requested a review from 1rneh June 3, 2026 14:19
@victhorlopez

Copy link
Copy Markdown
Contributor Author

Apologies for the noise... I had to disable yet another one browser/base/content/test/sync/browser_sendtab_telemetry.js
Maybe this one deserves some other bug (at least so it can be assessed later on whether badge menu needs to have similar metrics). It is about glean metrics with FxA interaction

  • Glean.fxaAvatarMenu.sendTabExposed "send tab" feature shown in the FxA avatar menu
  • Glean.fxaAvatarMenu.sendTabOpened send tab submenu opened from avatar menu
  • Glean.fxaAvatarMenu.clickSendTab user clicked a device in the avatar menu
  • Glean.fxaAppMenu.sendTabExposed / .sendTabOpened / .clickSendTab same events but from the hamburger menu

@victhorlopez victhorlopez assigned 1rneh and unassigned victhorlopez Jun 8, 2026
@1rneh
1rneh removed their request for review June 9, 2026 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch:main PR that should be merged on enterprise-main branch test-greening Fixes failing or flaky tests to restore passing CI status without changing core application logic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants