705269 - Mozmill endurance test for back and forward navigation

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

Create a Mozmill Endurance test for navigating back and forward.
The test will look like this:
Setup Module
  Open page 1
  Open page 2
Main Test Checkpoints
  Click back button
  Click forward button
Teardown Module
  Force close tabs

Comment 1

[Remus Pop (:RemusPop)]

Attachment: patch v1

Used 2 pages from the mozmill-tests repo.

Not sure if we need to wait for page load after a back or a forward action. waitForPageLoad does not work for back and forward.

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 576930 [details] [diff] [review]
patch v1

Some nits here: 
>+
>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
>+const LOCAL_TEST_PAGES = [
>+  LOCAL_TEST_FOLDER + 'layout/mozilla_community.html',
>+  LOCAL_TEST_FOLDER + 'layout/mozilla_contribute.html'];
>+

Fix indentation please 

const LOCAL_TEST_PAGES = [
  LOCAL_TEST_FOLDER + 'layout/mozilla_community.html',
  LOCAL_TEST_FOLDER + 'layout/mozilla_contribute.html'
];


>+
>+  enduranceManager = new endurance.EnduranceManager(controller);
>+
>+  tabBrowser = new tabs.tabBrowser(controller);
>+

No need for a new line here

And yes, we do not need a waitForPageLoad when in comes to back and fwd. 
Those events are using a sandbox from what I remember - but was ages ago 
when looked into the Firefox code. 

Check the mxr for sanity, but I'm pretty sure I'm right.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 576930 [details] [diff] [review]
patch v1

>+function testNavigateBackForward() {
>+  enduranceManager.run(function () {
>+    controller.goBack();
>+    enduranceManager.addCheckpoint("First webpage loaded");
>+
>+    controller.goForward();
>+    enduranceManager.addCheckpoint("Second webpage loaded");
>+  });
>+}

Could we change these checkpoint messages to relate to the navigation action that has just occurred. Something like: "Navigated back one page" / "Navigated forward one page"

Comment 4

[Remus Pop (:RemusPop)]

Is content.document.readyState going to suffice? This shows "complete" when the page loaded. I would want to test this with a very large webpage that does not load any url when called in a back-button action. 
The solution proposed works for youtube, but I think youtube does some GET actions when it is called with the back button.

Another solution would be to waitFor some element in the page.

Dave which one do you think would suite us here?

The messages will be changed on my next patch.

Comment 5

[Remus Pop (:RemusPop)]

document.readyState does not work anymore after back is clicked, so we cannot use it here. We need to find another method. Does anyone have any idea?

The error that I get for document.readyState after navigating back is Permission denied to access property 'document'

Comment 6

[u279076]

(In reply to Remus Pop (:RemusPop) from comment #5)
> document.readyState does not work anymore after back is clicked, so we
> cannot use it here. We need to find another method. Does anyone have any
> idea?
> 
> The error that I get for document.readyState after navigating back is
> Permission denied to access property 'document'

Is this a Firefox regression?

Comment 7

[Remus Pop (:RemusPop)]

I don't think that. There is another event that fires when we load content from bfcache (back and forward cache). I just thought that variable would help us when navigating back or forward.
Currently I'm investigating what variable change does occur when that event fires so we can use it in this test to know when the page loaded from cache.

Comment 8

[Remus Pop (:RemusPop)]

There might not be a variable change when the "pageshow" event fires. I think we need an event listener to get the event.

Please share your opinions here if we need a new bug to implement this in mozmill code (namely, controller.back() function for the back navigation action) or in a shared module or just add an event listener to the test.

Comment 9

[u279076]

Remus, can you first check the mozilla-central source code to see what developers are doing to test the back/forward navigation code? It would be helpful to have that as an example before moving forward with a decision.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Support for bfcache in waitForPageLoad() has been added with Mozmill 2, but it's not available in Mozmill 1.5.x. I would propose to use waitForElement for now.

Comment 11

[Remus Pop (:RemusPop)]

Attachment: patch v2

*changed the visited pages
*added waitForElement when navigating
*changed the checkpoint messages

Comment 12

[Remus Pop (:RemusPop)]

Attachment: patch v3

v2 did not use entities at all. Also added a delete call for element at the end of each entity.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 580887 [details] [diff] [review]
patch v3

># HG changeset patch
># User Remus Pop <[e-mail]>
># Date 1322233569 -7200
># Node ID f83a0bbd968f9c53df99fcd772ecd714c535dcba
># Parent  393c3847303e5f1a0e5483c2b6730d28984d7299
>Bug 705269 - Mozmill endurance test for back and forward navigation r=vlad.maniac, r=dave.hunt
>
>diff --git a/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js b/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js
>deleted file mode 100644
>--- a/tests/endurance/testAddons_OpenAndCloseAddonManager/test1.js
>+++ /dev/null The Original Code is Mozmill Test code.

Please check your patches. This one appears to remove all endurance tests...

>+function teardownModule() {
>+// tabBrowser.closeAllTabs();
>+}

Why is this line commented out?

>+function testNavigateBackForward() {
>+  enduranceManager.run(function () {
>+    controller.goBack();
>+    enduranceManager.addCheckpoint("First webpage loaded");
>+
>+    controller.goForward();
>+    enduranceManager.addCheckpoint("Second webpage loaded");
>+  });
>+}

See my comment 3 about changing these messages.

You mentioned adding entities to this test, however I do not see them in this patch. I also do not see any value in adding entities to this test.

Comment 14

[Remus Pop (:RemusPop)]

Attachment: patch v4

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 580900 [details] [diff] [review]
patch v4


># Parent  393c3847303e5f1a0e5483c2b6730d28984d7299
>Bug 705269 - Mozmill endurance test for back and forward navigation r=vlad.maniac, r=dave.hunt

Anthony Hughes is your second review-er 

>+
>+      delete element;

Please add a comment to explain briefly why are you clearing the memory here. I would not recommend doing it here, but in teardownModule 
if you really need to. If you want to clear memory from what you've stored in with elementslib, you should first check what 
elementlib does - investigate how it works

Comment 16

[Remus Pop (:RemusPop)]

Do we need the delete operator to delete the newly created element after each use given that we might have 100 or maybe 1000 iterations? When does gc kick in? Would this affect memory usage in Firefox and not show correct memory readings because we have 100 elements in the memory?

Comment 17

[Remus Pop (:RemusPop)]

Attachment: patch v5

Deleted the part with memory freeing as gc is working fine.
Updated the commit message.

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 587305 [details] [diff] [review]
patch v5

nit: The same mistake I did in a patch - the year in the MPL license 2012 please 
>+
>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
>+const LOCAL_TEST_PAGES = [
>+  {url: LOCAL_TEST_FOLDER + 'layout/mozilla.html', id: 'community'},
>+  {url: LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html', id: 'mission_statement'}
>+]

We need to have ";" after the "]" statement
>+
>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+
>+  enduranceManager = new endurance.EnduranceManager(controller);
>+  tabBrowser = new tabs.tabBrowser(controller);
>+
>+  tabBrowser.closeAllTabs();
>+
>+  // Open the test pages
>+  controller.open(LOCAL_TEST_PAGES[0].url);
>+  controller.waitForPageLoad();
>+  controller.open(LOCAL_TEST_PAGES[1].url);
>+  controller.waitForPageLoad();
Please add a forEach - it will not be that optimized but at least we'll be consistent with other tests 
and its more elegant code-wise than duplicating statements. 

>+    enduranceManager.loop(function () {
>+
>+      // Go back one page
We don't need a blank space here. Please remove

With those changes you have my r+

Comment 19

[Remus Pop (:RemusPop)]

Attachment: patch v6

All requested changes were made.

Comment 20

[Remus Pop (:RemusPop)]

The test is using controller.goBack() and goForward() functions. Should we click on the buttons instead? If yes, we should have a fix for bug 704140. The same fix will be applied here. So maybe we should have a separate bug and add a waitFor to the API.

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Remus Pop (:RemusPop) from comment #20)
> The test is using controller.goBack() and goForward() functions. Should we
> click on the buttons instead? If yes, we should have a fix for bug 704140.
> The same fix will be applied here. So maybe we should have a separate bug
> and add a waitFor to the API.

Ideally we will want to click, to simulate the user action - but then again we can have a checked in patch using backend API if something is blocking us, as we did in other tests. 

Just make sure you add a //XXX: to explain why we are using controller.goBack() .. instead of clicking on the button - this is imminent for the patch to contain so we can go back in the near future and update. 

I am going to cancel the review because I don't see that aspect in the patch right now. Remus please update then ask for r again - it will be a +

Comment 22

[Remus Pop (:RemusPop)]

Attachment: test v7 [landed]

Added the requested comment.

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 588432 [details] [diff] [review]
test v7 [landed]

It's a + from me. Moving over to Anthony, though I think it will need a little 
code style change, I won't hold this patch for those aspects IMO. 

Anthony will make the final decisions. Test is working as expected and data 
is gathered properly.

Comment 24

[u279076]

Comment on attachment 588432 [details] [diff] [review]
test v7 [landed]

This looks fine to me, though I would like to see Dave's feedback before landing it. Dave, does this satisfy the requirements of the test in your opinion?

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 588432 [details] [diff] [review]
test v7 [landed]

Looks good to me.

Comment 26

[u279076]

Comment on attachment 588432 [details] [diff] [review]
test v7 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/42a982378bd1 (default)

Please check tomorrow's testrun to ensure the test is not causing any regressions.

Comment 27

[Remus Pop (:RemusPop)]

Hasn't failed since checked in:
http://mozmill-release.blargon7.com/#/endurance/reports?branch=13.0&platform=All&from=2012-02-07&to=2012-02-10
Can be checked in other branches.
So marking this as VERIFIED.

Comment 28

[u279076]

Comment on attachment 588432 [details] [diff] [review]
test v7 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/829eb7f3fc87 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/49cea842da76 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/819e3aa37289 (mozilla-release)

NOTE: We normally don't mark a bug VERIFIED until a patch has landed on all affected branches.