Mozmill endurance test for back and forward navigation

VERIFIED FIXED

Status

defect
VERIFIED FIXED
8 years ago
2 months ago

People

(Reporter: remus.pop, Assigned: remus.pop)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-endurance])

Attachments

(1 attachment, 6 obsolete attachments)

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
Status: NEW → ASSIGNED
No longer blocks: 629065
Posted patch patch v1 (obsolete) — Splinter Review
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.
Attachment #576930 - Flags: review?(vlad.mozbugs)
Blocks: 629065
Whiteboard: [mozmill-endurance]
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.
Attachment #576930 - Flags: review?(vlad.mozbugs) → review-
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"
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.
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'
(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?
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.
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.
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.
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.
Posted patch patch v2 (obsolete) — Splinter Review
*changed the visited pages
*added waitForElement when navigating
*changed the checkpoint messages
Attachment #576930 - Attachment is obsolete: true
Attachment #579042 - Flags: review?(vlad.mozbugs)
Posted patch patch v3 (obsolete) — Splinter Review
v2 did not use entities at all. Also added a delete call for element at the end of each entity.
Attachment #579042 - Attachment is obsolete: true
Attachment #579042 - Flags: review?(vlad.mozbugs)
Attachment #580887 - Flags: review?(vlad.mozbugs)
Comment on attachment 580887 [details] [diff] [review]
patch v3

># HG changeset patch
># User Remus Pop <remus.pop@softvision.ro>
># 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.
Posted patch patch v4 (obsolete) — Splinter Review
Attachment #580887 - Attachment is obsolete: true
Attachment #580887 - Flags: review?(vlad.mozbugs)
Attachment #580900 - Flags: review?(vlad.mozbugs)
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
Attachment #580900 - Flags: review?(vlad.mozbugs) → review-
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?
Posted patch patch v5 (obsolete) — Splinter Review
Deleted the part with memory freeing as gc is working fine.
Updated the commit message.
Attachment #580900 - Attachment is obsolete: true
Attachment #587305 - Flags: review?(vlad.mozbugs)
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+
Attachment #587305 - Flags: review?(vlad.mozbugs) → review-
Posted patch patch v6 (obsolete) — Splinter Review
All requested changes were made.
Attachment #587305 - Attachment is obsolete: true
Attachment #588333 - Flags: review?(vlad.mozbugs)
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.
(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 +
Attachment #588333 - Flags: review?(vlad.mozbugs)
Added the requested comment.
Attachment #588333 - Attachment is obsolete: true
Attachment #588432 - Flags: review?(vlad.mozbugs)
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.
Attachment #588432 - Flags: review?(vlad.mozbugs)
Attachment #588432 - Flags: review?(anthony.s.hughes)
Attachment #588432 - Flags: review+
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?
Attachment #588432 - Flags: review?(anthony.s.hughes)
Attachment #588432 - Flags: review+
Attachment #588432 - Flags: feedback?(dave.hunt)
Comment on attachment 588432 [details] [diff] [review]
test v7 [landed]

Looks good to me.
Attachment #588432 - Flags: feedback?(dave.hunt) → feedback+
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.
Attachment #588432 - Attachment description: patch v7 (all branches) → patch v7 (all) [landed:default]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
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.
Attachment #588432 - Attachment description: patch v7 (all) [landed:default] → test v7 [landed]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.