607139 - Clean up sessionstore test suite

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Suffice it to say that the sessionstore tests are a bit of a mess. We've got pretty good test coverage now (at least in comparison to a few years ago).

However one of the things that bothers me is that there is a lot of copy-pasta. We end up doing the same thing in 20 different ways (set state and wait for it, open a window & wait for it, count windows, etc).

Some point recently we got the capability to have a head.js per test suite. We should take a close look at what's being done repeatedly and take advantage of this, make some utility functions, and just clean things up.

It might also be good to "finish" bug 426045 and make sure we have good coverage for existing functionality. Like I said, we're better but I've found at least one thing recently that I expected to be covered by tests but wasn't.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

I would also suggest adding a TestRunner like some other test suites already have. I included this in the newtab test suites and it makes async code so much more readable. The idea is to override the actual test() function and provide an iterator that yields when it's waiting for some async event to occur. So there's no callback hassle and indentation mess. Example:

function runTests() {
  yield addBlankTabAndWaitUntilsItHasLoaded();
  
  // do otherstuff ...
  yield waitForTabState(tab, tabState);
  info("yay, tab state");

  // continue on a fresh tick
  yield executeSoon(next);
}

Can be implemented like this:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/newtab/head.js#27

Comment 2

[Andres Hernandez [:andreshm]]

Attachment: Current progress

This patch is not ready yet.
Bellindira will continue with this bug, complete the patch and include the testRunner.

Comment 3

[Bellindira Castillo [:bellindira]]

Attachment: Current progress

Attached is a new version which contains the current progress of the bug.
This is intended to show a preview because not all the test cases are stable. However, you can check the current use of TestRunner on the module, and run some of the testcases but not all, due to the migration of the sessionstore module to the new test suite is still on progress.
If you have any question, please let me know.
Thanks, 
Bellindira

Comment 4

[Bellindira Castillo [:bellindira]]

Comment on attachment 634807 [details] [diff] [review]
Current progress

Currently, I am working on refactor all the tests (I must go one by one) to use 'yield' (generator) instead of the callbacks to make it more readable. Also, I added methods as whenBrowserLoaded and whenWindowLoaded to head because they are common on mostly of the tests.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 634807 [details] [diff] [review]
Current progress

Review of attachment 634807 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Bellindira (and Andres) for taking this on! I did a quick look over and overall I like what I see. Since I didn't get to know yield & generators very well, I do have one question... should any tests themselves be calling finish or should TestRunner be the only callsite?

When it's ready for review, feel free to ask me, but it will likely take some time. For a faster turnaround, Tim is probably a better person to ask.

::: .hgignore
@@ +45,5 @@
>  \.project$
>  \.cproject$
>  \.settings/
> +other-licenses
> +testing

make sure you don't include this in a final patch

::: browser/components/sessionstore/test/browser_formdata_format.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

A few points here about licenses...
1. Don't change the licenses on any files, especially not in a more restrictive direction.
2. I think once it's public domain, it's not actually legal to relicense in a more restrictive way (but I'm far from a lawyer)
3. I saw you added some licenses to files. That's probably ok and good to do. MPL2 is fine for those cases
4. We're trying to keep test files Public Domain as far as I know.

@@ +105,5 @@
>  
>        // force a change event to recollect the formdata
>        let changeEvent = document.createEvent("Events");
>        changeEvent.initEvent("change", true, true);
> +      input1.dispatchEvent(changeEvent);

good catch

@@ +110,4 @@
>      };
>  
> +    whenTabRestored(tab, tabStateCallback);
> +    ss.setTabState(tab, JSON.stringify(tabState));

I liked the waitForTabState syntax better... Is there a good reason to split this up into 2 steps? Even if it's split up in head.js for good reason, some sugar to keep tests a little cleaner is nice.

Comment 6

[Bellindira Castillo [:bellindira]]

Thanks for the feedback Paul.

Should any tests themselves be calling finish or should TestRunner be the only callsite?
The TestRunner should be the only one who call finish (even when it is possible to call it on the tests).

I'll remove all licenses changes of this patch. Because the idea was to remove the MPL 1.1/GPL 2.0/LGPL 2.1. as you can see on the Andres patch but then they were implemented on another Session Store patch.

I liked the waitForTabState syntax better... Is there a good reason to split this up into 2 steps?
Well, it is because sometimes setTabState is not use it whenTabRestored (e.g browser_483330.js or browser_739805.js). However, I agree, it is better to have only one step. So, I'll implement a waitForTabState function instead of whenTabRestored and it will handle if state does not need to be set.

Comment 7

[Bellindira Castillo [:bellindira]]

Attachment: This version is intended to show the current progress only.

Comment 8

[Bellindira Castillo [:bellindira]]

Attachment: Functional Patch

This is almost the final version of the patch. I am just working on fixing some leaks after running all the tests. So, you now can run all the tests of sessionstore module.

Comment 9

[Bellindira Castillo [:bellindira]]

Attachment: Patch

Added a TestRunner.
Refactored all the tests
Implemented fixes according to feedback.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 639782 [details] [diff] [review]
Patch

Clearing review for now. We're currently refactoring big chunks of session restore and it's not clear yet how tests will be affected and what we'll do with that patch. Thanks for all your work and sorry that I wasn't faster reviewing this.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Closing this. Tests will be refactored on a case by case basis as we move towards an e10s compatible sessionstore implementation.