Closed Bug 1383505 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file, 1 obsolete file)

Bug 1383505 - add test to make sure onboarding show the first uncomplete tour by default;
59 bytes, text/x-review-board-request
mossop
: review+
Details 
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
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [photon-onboarding]
Should revisit this bug after the bug 1383070
Depends on: 1383070
Priority: P3 → P5
(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
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Attachment #8893170 - Attachment is obsolete: true
Attachment #8893206 - Attachment is obsolete: true
Attachment #8893206 - Flags: review?(fliu)
Attachment #8893206 - Flags: review?(dtownsend)
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
Attachment #8893170 - Flags: review?(dtownsend) → review+
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.
Attachment #8893170 - Flags: review?(fliu)
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.
Attachment #8893170 - Flags: review?(fliu)
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
(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 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
Attachment #8893170 - Flags: review?(fliu)
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)
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)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/64c296a70dbf
add test to make sure onboarding show the first uncomplete tour by default;r=mossop
Keywords: checkin-needed
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).
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.
https://hg.mozilla.org/mozilla-central/rev/64c296a70dbf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: