696724 - Mozmill test for fastload XBL methods and properties

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

Attachment: Patch v2

This bug has been separated from bug 94199, which holds the base implementation of the feature. It makes more sense to discuss the implementation of the test in our own component instead under core/XBL.

Attached is the latest version of the patch from Neil.

Comment 1

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

Comment on attachment 569019 [details] [diff] [review]
Patch v2

>+++ b/lib/files.js
>+function readFromTempFile(filename)
>+function writeToTempFile(filename, output)

Those functions should be able to not only handle files in the temp folder. Can we make those more generic and pass in the full filename? We can just move out the TmpD retrieval code into the test and compute it there. You can also store the nsIFile object onto the persisted object, i.e. persisted.preOutputFile.

>+++ b/tests/functional/restartTests/testStartupXBLCache/test1.js
>+var setupModule = function() {
>+  var controller = mozmill.getBrowserController();

As long as you do not use controller, you can simply drop this line.

>+++ b/tests/functional/restartTests/testStartupXBLCache/test3.js
>+  var controller = mozmill.getBrowserController();

Same here.

>+  expect.equal(preOutput, postOutput, "Original non-cached content matches read-in cached content");

This is the final test and should be moved into a test function. Also this test will not work as long as we do not have fixed bug 683294, which should hopefully be done today or tomorrow.

>+function teardownModule(module) {
>+  var file = Components.classes["@mozilla.org/file/directory_service;1"].  
>+                        getService(Components.interfaces.nsIProperties).  
>+                        get("TmpD", Components.interfaces.nsIFile);  
>+  file.append("xblcache-pre-output.txt");

The persisted approach would save us a lot of code similar to this.

It would be great if you can make those changes. I can then follow-up and applying our style guides, if you don't have time to check for those:

https://developer.mozilla.org/en/Mozmill_Tests#Coding_style

Neil, further can you supply a tryserver build we can use to verify the functionality of this test? Thanks.

Comment 2

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

I will ensure that we can fix bug 683294 by today. Neil, you can keep the equal call because the deepEqual functionality will replace our current equal implementation.

Comment 3

[Neil Deakin]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> Those functions should be able to not only handle files in the temp folder.
> Can we make those more generic and pass in the full filename? We can just
> move out the TmpD retrieval code into the test and compute it there. You can
> also store the nsIFile object onto the persisted object, i.e.
> persisted.preOutputFile.
> 

I'd rather just change it to pass in the directory key, or just handle nsIFiles being passed in directly.


> >+++ b/tests/functional/restartTests/testStartupXBLCache/test1.js
> >+var setupModule = function() {
> >+  var controller = mozmill.getBrowserController();
> 
> As long as you do not use controller, you can simply drop this line.

It's used to get the window. Is there another way?

> >+  expect.equal(preOutput, postOutput, "Original non-cached content matches read-in cached content");
> 
> This is the final test and should be moved into a test function. Also this
> test will not work as long as we do not have fixed bug 683294, which should
> hopefully be done today or tomorrow.

Those are strings so equal should work fine, no?

I'm not sure what you mean by a 'test function'.

> >+function teardownModule(module) {
> >+  var file = Components.classes["@mozilla.org/file/directory_service;1"].  
> >+                        getService(Components.interfaces.nsIProperties).  
> >+                        get("TmpD", Components.interfaces.nsIFile);  
> >+  file.append("xblcache-pre-output.txt");
> 
> The persisted approach would save us a lot of code similar to this.

Not clear what you're referring to.

Comment 4

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

(In reply to Neil Deakin from comment #3)
> I'd rather just change it to pass in the directory key, or just handle
> nsIFiles being passed in directly.

Sounds also fine. That way the test could dynamically create the path on its own.

> > As long as you do not use controller, you can simply drop this line.
> 
> It's used to get the window. Is there another way?

Missed that. No, we should use the controller API even we could use a utils function of Mozmill.

> > >+  expect.equal(preOutput, postOutput, "Original non-cached content matches read-in cached content");
> > 
> > This is the final test and should be moved into a test function. Also this
> > test will not work as long as we do not have fixed bug 683294, which should
> > hopefully be done today or tomorrow.
> 
> Those are strings so equal should work fine, no?

I shouldn't do code reviews that late in the night. Yes, you are right here. It should work in any case.

> I'm not sure what you mean by a 'test function'.

Move this call into a function like testCheckXBLMethodsAndProperties.

> > The persisted approach would save us a lot of code similar to this.
> 
> Not clear what you're referring to.

The persisted object is used to transfer data between individual restart tests means Firefox processes. You will always have access to that data or code. See the following test how it can be used.

http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/restartTests/testRestartChangeArchitecture/

Comment 5

[Neil Deakin]

> Neil, further can you supply a tryserver build we can use to verify the
> functionality of this test? Thanks.

A build, which also addresses the review comments in the bug is at https://tbpl.mozilla.org/?tree=Try&rev=9e60f8f13136

Comment 6

[Neil Deakin]

(In reply to Henrik Skupin (:whimboo) from comment #4)

> The persisted object is used to transfer data between individual restart
> tests means Firefox processes. You will always have access to that data or
> code. See the following test how it can be used.
> 
> http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/
> restartTests/testRestartChangeArchitecture/

I understand what it is, but am not clear why you want me to use it. The data is over 1mb and you said in the other bug that it would be too large.

Comment 7

[Neil Deakin]

Attachment: Updated patch

Addresses the comments except as noted above.

Comment 8

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

(In reply to Neil Deakin from comment #5)
> A build, which also addresses the review comments in the bug is at
> https://tbpl.mozilla.org/?tree=Try&rev=9e60f8f13136

How do I get a link to the build itself?

Comment 9

[Neil Deakin]

You can get the builds by clicking one of the 'B' labels, and a link will appear at the bottom left. I did a slightly newer build, the more direct directory is at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-191791aafdce/

Comment 10

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

Comment on attachment 570898 [details] [diff] [review]
Updated patch

>+function readFromFile(file)
>+{

nit: in our coding styles we place the opening brackets in the same line. That would apply to any function definition in this patch.

>+  var fstream = Components.classes["@mozilla.org/network/file-input-stream;1"].  
>+                           createInstance(Components.interfaces.nsIFileInputStream);  
>+  var stream = Components.classes["@mozilla.org/intl/converter-input-stream;1"].  
>+                          createInstance(Components.interfaces.nsIConverterInputStream);  

Please make use of Cc and Ci everywhere.

>+var setupModule = function() {

In all of our new tests we use "function XYZ() {". Also please add the aModule parameter to the definition...

>+  var controller = mozmill.getBrowserController();
>+  var node = controller.window.document.documentElement;

so that you can assign the controller (and other variables) as 'aModule.controller'. 

>+  var file = Components.classes["@mozilla.org/file/directory_service;1"].  
>+                        getService(Components.interfaces.nsIProperties).  
>+                        get("TmpD", Components.interfaces.nsIFile);  
>+  file.append("xblcache-pre-output.txt");

What I meant by using persisted is that we should save off the file.path to it for re-usage in test3.js. That way we only would have to change this test module if the path has to be changed.

>+function filterNodes(node)
>+{
>+  var name = node.localName;
>+  if (name == "notificationbox" || name == "toolbarspring") {
>+    node.removeAttribute("id");

The removal of the existing notificationboxes can't be done in the filter callback?

>+var preFile, postOutput;

No need for global variables. Make those available via aModule.preFile and aModule.proFile.

>+var testCheckXBLMethodsAndProperties = function()
>+{
>+  preFile = Components.classes["@mozilla.org/file/directory_service;1"].  
>+                       getService(Components.interfaces.nsIProperties).  
>+                       get("TmpD", Components.interfaces.nsIFile);
>+  preFile.append("xblcache-pre-output.txt");
>+  var preOutput = files.readFromFile(preFile);

Those lines have to be part of setupModule.

Because I can't test this patch (see above) and given the remaining comments, we should have one more review round.

Comment 11

[Neil Deakin]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> >+  var controller = mozmill.getBrowserController();
> >+  var node = controller.window.document.documentElement;
> 
> so that you can assign the controller (and other variables) as
> 'aModule.controller'. 

I don't see what the point of doing that is.


> The removal of the existing notificationboxes can't be done in the filter
> callback?

Could do, but it would mean more work for every node rather than just removing at the beginning. And I want to make sure to only remove the notifications associated with the main tab rather than some other notificataion that could possibly be added.

Comment 12

[Neil Deakin]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> >+var preFile, postOutput;
> 
> No need for global variables. Make those available via aModule.preFile and
> aModule.proFile.

I can't get any of this to work. I can't find any existing test that does it this way. In fact, many tests use globals without even declaring them which is even worse.

Can you describe or point to some information about how this needs to be done?

Comment 13

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

Sorry for the delay but I was away the last days. Neil, would it be ok for you when I update your patch with the comments I have in mind? I think that would speed-up the process to get the test checked-in.

Comment 14

[Neil Deakin]

Attachment: patch in progress

> Neil, would it be ok for you when I update your patch with the comments
> I have in mind? I think that would speed-up the process to get the test
> checked-in.

Sure. Attached is what I had attempted.

Comment 15

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

Attachment: Updated patch

I have updated the patch regarding the review comments, except for the module parameter.

Neil, this test is always failing for me with the latest Nightly build on OS X. I have uploaded the stringified objects to my people account. I hope that helps you to identify the problem.

http://people.mozilla.com/~hskupin/downloads/

Comment 16

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

Neil, any updates from your side?

Comment 17

[Neil Deakin]

The test with your updated patch works well.

The test needs to run with '--app-arg=about:blank' such that the browser is opened with a blank page. Otherwise, the one time start page opens the first time.

Either the test needs to be set to run this way, or there needs to be way to disable the start page and load about:blank each time.

Comment 18

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

We cannot pass arguments for the application into the testrun. That why the test has to prepare itself correctly. Given your above initial state is there anything else we have to take care of? Personally I would say the pre-state should be:

* Only a single tab is open
* about:blank is loaded
* no notification bar is visible

Do we also have to set the sessionstore pref so about:blank gets reopened automatically after a restart?

Comment 19

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

Attachment: Patch v3 (single tab, restore previous session)

Neil, it's still not enough to close all tabs, load about:blank, and ensure that we restore the previous session on start-up. Could it be that the string conversion produces different results? I haven't compared the output yet given by that version of the patch.

Comment 20

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

Oh, and after the first restart there is one more notification bar displayed. It's about performance data. Not sure if this somehow interferes here.

Comment 21

[Neil Deakin]

Opening tabs and then closing them doesn't produce the same result as not opening any to begin with. Various bits of state will be updated differently (menu disabled state, status labels, etc). Also, I believe the closed tabs are in a state that they can be reopened with undo.

I tried the same thing you did, but the complexity and differences are too great. I'd rather have the browser open in exactly the same manner so as to test with as minimal variations as possible.

Comment 22

[Neil Deakin]

Attachment: Update patch handling first run page

This patch should fix the first-run page issue by:

- moving the work from test1.js into test2.js
- just to ensure the cache doesn't exist, test1.js deletes the cache file.

Comment 23

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

Neil, how do you run the test? It's still failing for me. You will have to execute Mozmill like:

mozmill-restart -b /Applications/Nightly.app/ --show-errors -t tests/functional/restartTests/testStartupXBLCache/

Comment 24

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

Comment on attachment 579678 [details] [diff] [review]
Update patch handling first run page

It's still failing for me. Not sure if this is the performance notification bar which gets displayed in the second test module.

Comment 25

[Neil Deakin]

Henrik, do you want to try it with the notification removing code added back in?

If that doesn't work, could you send me the differences between what the two calls to serializeTree return?

Comment 26

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

Neil, sorry for the missing reply. It somehow got under the wire. Would such a test still be valuable? Mozmill is dead but we could integrate it via Marionette in our Firefox UI Tests.

Comment 27

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

(In reply to Henrik Skupin (:whimboo) from comment #26)
> Neil, sorry for the missing reply. It somehow got under the wire. Would such
> a test still be valuable? Mozmill is dead but we could integrate it via
> Marionette in our Firefox UI Tests.

Missed to set needinfo for Neil. Doing it now so we have an idea if it would still be helpful or not.

Comment 28

[Neil Deakin]

I don't think we need a test for this