1383505 - add test to make sure onboarding show the first uncomplete tour by default

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P5)

Description

[[e-mail]]

As bug 1381765 comment 19 we'd prefer land test after fixing intimittent issues around tests.


Here's the origin test which need to replace 'onboarding-overlay-icon' to 'onboarding-overlay-button'

https://reviewboard.mozilla.org/r/159090/diff/6#index_header

Comment 1

[Fischer [:Fischer]]

Should revisit this bug after the bug 1383070

Comment 2

[Fischer [:Fischer]]

(In reply to Fred Lin [:gasolin] from comment #0)
> As bug 1381765 comment 19 we'd prefer land test after fixing intimittent
> issues around tests.
> 
> 
> Here's the origin test which need to replace 'onboarding-overlay-icon' to
> 'onboarding-overlay-button'
> 
> https://reviewboard.mozilla.org/r/159090/diff/6#index_header
Some items to notice, thanks:

- The bug 1383070 has introduced 2 utility functions: `openTab` and `reloadTab` to reduce repeated codes and to make sure we proceed before these tasks get done.

- We might want to run 10~20 tries on Linux to make sure we don't hit the test-running-too-long issue. Per the bug 1383070 comments [1][2][3], the loading time is a bit long.


[1] bug 1383070 comment 4
[2] bug 1383070 comment 10
[3] bug 1383070 comment 28

Comment 3

[[e-mail]]

Attachment: Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

Review commit: https://reviewboard.mozilla.org/r/164174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164174/

Comment 4

[[e-mail]]

Attachment: Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

Review commit: https://reviewboard.mozilla.org/r/164240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164240/

Comment 5

[[e-mail]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164174/diff/1-2/

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/164174/#review169944

Comment 7

[Fischer [:Fischer]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/164174/#review170148

::: browser/extensions/onboarding/test/browser/browser_onboarding_select_default_tour.js:28
(Diff revision 2)
> +  await BrowserTestUtils.synthesizeMouseAtCenter(OVERLAY_ICON_ID, {}, tab.linkedBrowser);
> +  await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> +
> +  let doc = content && content.document;
> +  let dom = doc.querySelector(PRIVATE_BROWSING_TOUR_ID);
> +  ok(dom.classList.contains(CLASS_ACTIVE));

overall looks good. Only need to add the assertion comment such as `ok(dom.classList.contains(CLASS_ACTIVE), "Should/Expect ....")` so that others could know what's the test purpose clearly and know what's wrong in the 1st place if Try fails.

Comment 8

[[e-mail]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164174/diff/2-3/

Comment 9

[Fischer [:Fischer]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/164174/#review171074

::: browser/extensions/onboarding/test/browser/browser_onboarding_select_default_tour.js:28
(Diff revision 3)
> +  await BrowserTestUtils.synthesizeMouseAtCenter(OVERLAY_ICON_ID, {}, tab.linkedBrowser);
> +  await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> +
> +  info("Make sure the default tour is selected");
> +  let doc = content && content.document;
> +  let dom = doc.querySelector(PRIVATE_BROWSING_TOUR_ID);

Better to use `getCurrentActiveTour` which will return { activeNavItemId, activePageId } so can check both the page and the nav item. After this update, I think we are good to go.

::: browser/extensions/onboarding/test/browser/browser_onboarding_select_default_tour.js:31
(Diff revision 3)
> +  info("Make sure the default tour is selected");
> +  let doc = content && content.document;
> +  let dom = doc.querySelector(PRIVATE_BROWSING_TOUR_ID);
> +  ok(dom.classList.contains(CLASS_ACTIVE), "default tour is selected");
> +  let dom2 = doc.querySelector(ADDONS_TOUR_ID);
> +  ok(!dom2.classList.contains(CLASS_ACTIVE), "none default tour should not be selected");

Should we check both the page and the nav item for the "not" case, it is up to you. The current approach is fine.

Comment 10

[[e-mail]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164174/diff/3-4/

Comment 11

[[e-mail]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/164174/#review171542

::: browser/extensions/onboarding/test/browser/browser_onboarding_select_default_tour.js:28
(Diff revision 3)
> +  await BrowserTestUtils.synthesizeMouseAtCenter(OVERLAY_ICON_ID, {}, tab.linkedBrowser);
> +  await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> +
> +  info("Make sure the default tour is selected");
> +  let doc = content && content.document;
> +  let dom = doc.querySelector(PRIVATE_BROWSING_TOUR_ID);

for these tests we'd better cover both active and non active tours, and `getCurrentActiveTour` only take care active items, which was covered by CLASS_ACTIVE in these test cases.

I'll add a separate test case that use getCurrentActiveTour to test the default tour open the right tour page

Comment 12

[Fischer [:Fischer]]

(In reply to Fred Lin [:gasolin] from comment #11)
> Comment on attachment 8893170 [details]
> Bug 1383505 - add test to make sure onboarding show the first uncomplete
> tour by default;
> 
> https://reviewboard.mozilla.org/r/164174/#review171542
> 
> :::
> browser/extensions/onboarding/test/browser/
> browser_onboarding_select_default_tour.js:28
> (Diff revision 3)
> > +  await BrowserTestUtils.synthesizeMouseAtCenter(OVERLAY_ICON_ID, {}, tab.linkedBrowser);
> > +  await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> > +
> > +  info("Make sure the default tour is selected");
> > +  let doc = content && content.document;
> > +  let dom = doc.querySelector(PRIVATE_BROWSING_TOUR_ID);
> 
> for these tests we'd better cover both active and non active tours, and
> `getCurrentActiveTour` only take care active items, which was covered by
> CLASS_ACTIVE in these test cases.
> 
Don't get what you mean. The improvement has nothing to do with you comment here. The `getCurrentActiveTour` returns { activeNavItemId, activePageId } so we could check both page and nav item with one line of code change.
And the comment 9 said the current "not" case check is fine, not saying remove the "not" case or more updates. Up to you to check more about nav item.

Comment 13

[Fischer [:Fischer]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/164174/#review171546

Comment 14

[[e-mail]]

Comment on attachment 8893170 [details]
Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164174/diff/4-5/

Comment 15

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)

Comment 16

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64c296a70dbf
add test to make sure onboarding show the first uncomplete tour by default;r=mossop

Comment 18

[Fischer [:Fischer]]

Not a good practice to request a checkin-needed after a r? cancel. Consider you don't really pay attention what is being done, for example, in the previous bug 1381765, an explicit rebasing issue was ignored without being taken care (bug 1381765 comment 20).

Comment 19

[[e-mail]]

Sorry the reason why not to change is missing (not posted) by accident, and make you feel not good :/ 

The current tests already cover both active and not-active test cases,
and `getCurrentActiveTour` brings just a little value for those test cases but increasing the test time, so I'd rather add an extra test instead of update those tests. We can add more tests later if necessary.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/64c296a70dbf

Comment 21

[Fischer [:Fischer]]

(In reply to Fred Lin [:gasolin] from comment #19)
> Sorry the reason why not to change is missing (not posted) by accident, and
> make you feel not good :/ 
> 
> The current tests already cover both active and not-active test cases,
> and `getCurrentActiveTour` brings just a little value for those test cases
> but increasing the test time, so I'd rather add an extra test instead of
> update those tests. We can add more tests later if necessary.
In fact the increasing time should not be huge and adding another test also increases time as well. One of the advantage of reusing `getCurrentActiveTour` is that we have a consistent way doing that query and only have to update one function if we make change on CSS in the future. Anyway, just leave it since got checked-in.