486342 - Write a browser-chrome test suite for the various "Save As" functions in Toolkit's "contentAreaUtils.js"

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement)

Description

[:Paolo Amadini]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)

The various "Save As" commands, available from the browser's context menus
and other places, use some JavaScript functions that are defined in the
"contentAreaUtils.js" file. For Firefox, the file is located under the
"toolkit" folder.

These functions need a test suite that exercises all the various save
scenarios. Such suite would ideally test all the possible code paths,
checking things such as the name used by default for the target file,
the available save types (complete, HTML only, ...) based on the MIME
types of the content, possible error conditions with the save operation,
and so on.

Reproducible: Always




SeaMonkey uses a separate "contentAreaUtils.js" file, located under the "suite" folder. To be useful for SeaMonkey development too, the test cases should be
written in a way that differentiates between the expected behavior for the two
products, if different.

Ideally, the common functions should be moved to a shared place where they can
be tested in the same way for both products. This is covered by bug 484616.

Comment 1

[:Paolo Amadini]

From some source code comments in "contentAreaUtils.js":

// Try saving each of these types:
// - A complete webpage using File->Save Page As, and Context->Save Page As
// - A webpage as HTML only using the above methods
// - A webpage as Text only using the above methods
// - An image with an extension (e.g. .jpg) in its file name, using
//   Context->Save Image As...
// - An image without an extension (e.g. a banner ad on cnn.com) using
//   the above method.
// - A linked document using Save Link As...
// - A linked document using Alt-click Save Link As...

Comment 2

[:Paolo Amadini]

Attachment: Work in progress - Test suite in a single file

This is a first attempt at generalizing the test functions for "Save As".
The "browser_save.js" file is a copy of "browser_bug471962.js", with more
code moved to support functions, more generalized mock objects, and an
approach based on generator functions that allows the test procedures to
be more robust using try...finally constructs.

One immediately perceivable advantage of this new test case, compared to
"browser_bug471962.js", is that in case of exceptions it does not time out
and performs a complete cleanup.

The final goal is to move most of the code to reusable support functions
to ease the process of writing a complete test suite for all the "Save As"
cases.

At present I'm interested in thoughts on this approach and in suggestions,
more than a thorough review.

Some specific questions I have at present:
 * Is there a way, in a makefile, to copy from the source tree to the test
    tree all the files in all the subdirectories of a "data" folder? Is
    this advisable? The "data" folder would contain a small "website" with
    various HTML, CSS, images, and other content files to be saved by the
    tests. The files would be located at various depth levels.
 * What may be the best way to share the support functions between different
    browser chrome test files? Doing this would prevent the test suite from
    turning into one big monolithic file over time, in addition to providing
    separation between the infrastructure and the real tests.

Comment 3

[Shawn Wilsher :sdwilsh]

It's going to be about a week before I'll be able to look at this patch.  Just wanted you to know there was going to be a bit of delay here.

Comment 4

[:Paolo Amadini]

(In reply to comment #3)
> It's going to be about a week before I'll be able to look at this patch.  Just
> wanted you to know there was going to be a bit of delay here.

Thanks for the notification! I'll wait for your comments.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> One immediately perceivable advantage of this new test case, compared to
> "browser_bug471962.js", is that in case of exceptions it does not time out
> and performs a complete cleanup.
great!

> The final goal is to move most of the code to reusable support functions
> to ease the process of writing a complete test suite for all the "Save As"
> cases.
Good!

>  * Is there a way, in a makefile, to copy from the source tree to the test
>     tree all the files in all the subdirectories of a "data" folder? Is
>     this advisable? The "data" folder would contain a small "website" with
>     various HTML, CSS, images, and other content files to be saved by the
>     tests. The files would be located at various depth levels.
See how http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/Makefile.in does utils.js

>  * What may be the best way to share the support functions between different
>     browser chrome test files? Doing this would prevent the test suite from
>     turning into one big monolithic file over time, in addition to providing
>     separation between the infrastructure and the real tests.
do you need browser chrome?  using just a plain chrome test is a bit easier to do this.  I can't think of a way offhand to do it with browser chrome.

Comment 6

[Shawn Wilsher :sdwilsh]

In general, I think the approach here is good.  You may want to consider writing your own test running for this in a chrome test though (it's worth considering at least).  It'd just run through a list of files that contain the test functions, and you can provide all the magic you need to for them.

Thanks a ton for doing this!

Comment 7

[:Paolo Amadini]

(In reply to comment #5)
> (In reply to comment #2)
> >  * Is there a way, in a makefile, to copy from the source tree to the test
> >     tree all the files in all the subdirectories of a "data" folder? Is
> >     this advisable? The "data" folder would contain a small "website" with
> >     various HTML, CSS, images, and other content files to be saved by the
> >     tests. The files would be located at various depth levels.
> See how
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/Makefile.in
> does utils.js

In that makefile, I see a list of files that are explicitly named and located
at the same level in the tree. What I'd like to know is if there is a way
to copy a folder and all its subfolders without the need to name every file
explicitly, and if that can be advisable. I'm asking the latter because maybe
the build system allows that kind of copy, but can't handle it efficiently
for whatever reason. This is an example of a tree that may be copied this way:

 data/
  first-file-to-save-referencing-the-images.html
  second-file-to-save-referencing-the-images.html
  stylesheets/
   ...
  images/
   served-without-content-type/
    image.jpg
    image.jpg^headers^
    image.gif
    image.gif^headers^
    ...
   served-with-wrong-content-type/
    image.jpg
    image.jpg^headers^
    image.gif
    image.gif^headers^
    ...
   many-other-folders/
    ...

(In reply to comment #6)
> In general, I think the approach here is good.  You may want to consider
> writing your own test running for this in a chrome test though (it's worth
> considering at least).  It'd just run through a list of files that contain the
> test functions, and you can provide all the magic you need to for them.

Some time ago we found out that browser-chrome was the way to go to test the
save functions that are loaded in the browser's window (bug 471962, comments
20 and 21). As for sharing code in browser-chrome, maybe I can include the
common script directly from every other test using its "chrome" URL and the
mozIJSSubScriptLoader component. I think the subscript loader could be used
in the other direction too (a master file that loads and executes subtests),
but I'm not sure if I can make the support functions available to the subtests
in the proper namespace. Any suggestion or prior art?

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> In that makefile, I see a list of files that are explicitly named and located
> at the same level in the tree. What I'd like to know is if there is a way
> to copy a folder and all its subfolders without the need to name every file
> explicitly, and if that can be advisable. I'm asking the latter because maybe
> the build system allows that kind of copy, but can't handle it efficiently
> for whatever reason. This is an example of a tree that may be copied this way:
I don't think there is an easy way to do that.  Sorry!

> Some time ago we found out that browser-chrome was the way to go to test the
> save functions that are loaded in the browser's window (bug 471962, comments
> 20 and 21). As for sharing code in browser-chrome, maybe I can include the
> common script directly from every other test using its "chrome" URL and the
> mozIJSSubScriptLoader component. I think the subscript loader could be used
> in the other direction too (a master file that loads and executes subtests),
> but I'm not sure if I can make the support functions available to the subtests
> in the proper namespace. Any suggestion or prior art?
No prior art, but that may work too.

Comment 9

[:Paolo Amadini]

Attachment: Work in progress - Test suite in multiple files

Sorry for the long wait, the good news is that the work here is near to
completion. I reorganized most support functions in reusable objects,
each in its own file. The main test file, "browser_save.js", is now
limited to the test case itself.

Let me know if this sounds good to you. "browser_save.js" is the only
file in the patch that is not in its intended final version; it contains
a port of "browser_bug471962.js" and another example test. If the other
files are OK, I think the next step may be to modify "browser_bug471962.js"
to use the new test support objects and check-in, before adding new tests.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 383094 [details] [diff] [review]
Work in progress - Test suite in multiple files

A few comments:
I much prefer using named functions and then iterating over an array of references to them when running a bunch of tests (like what you might see in http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/test_js_helpers_prototype_chain_safe.js).  I find it much easier to actually see what is being tested in each subtest.
It's not clear to me why so many function calls are wrapped in a try-catch-finally block.  Can you please add comments explaining things?
I'd like to see detailed javadoc comments on your helper methods please (including @param bits for arguments).

Comment 11

[:Paolo Amadini]

Attachment: Work in progress - Test suite in multiple files (no test queuing)

(In reply to comment #10)
> (From update of attachment 383094 [details] [diff] [review])
> A few comments:
> I much prefer using named functions and then iterating over an array of
> references to them when running a bunch of tests (like what you might see in
> http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/test_js_helpers_prototype_chain_safe.js).
>  I find it much easier to actually see what is being tested in each subtest.

The new testRunner object in this patch has a runTests method that accepts an
array, like you're suggesting. I also removed the ability to enqueue tests
using addTest and "start", since this feature would be unused.

> It's not clear to me why so many function calls are wrapped in a
> try-catch-finally block.  Can you please add comments explaining things?

If you're referring to the try-catch block themselves, I added some comments
explaining some passages. I think a general explanation of how things work
may be useful too, especially if some of these objects will be moved to
a more general place, but I also think this may deferred. If there is
something specific you didn't understand at a glance, and would like to see
explained in a comment, please tell me; an opinion from someone who
didn't write the code is valuable for determining what needs a better
explanation and what instead is generally obvious.

> I'd like to see detailed javadoc comments on your helper methods please
> (including @param bits for arguments).

Did this too. I tried to add small pieces of information in the process;
I really can't make myself write descriptions like "@param aSaveFunction
The save function." :-)

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 384146 [details] [diff] [review]
Work in progress - Test suite in multiple files (no test queuing)

The approach here seems fine.  I'll review the final patch.

Comment 13

[:Paolo Amadini]

(In reply to comment #12)
> (From update of attachment 384146 [details] [diff] [review])
> The approach here seems fine.  I'll review the final patch.

Thanks :-) I think I'll be able to provide it tomorrow or this week-end.

If it's not a problem, I'll give the name "browser_save_post_data.js" to
the new implementation of "browser_bug471962.js" and move the .sjs files
used by the test to the "data" folder.

Comment 14

[:Paolo Amadini]

Attachment: Test support library and one test for POST data [has renames]

Here is the final patch, including the rewritten test for bug 471962.

I verified that the new test fails if the patch for bug 471962 is reverted.

Comment 15

[:Paolo Amadini]

Comment on attachment 386827 [details] [diff] [review]
Test support library and one test for POST data [has renames]

(In reply to comment #14)
> Created an attachment (id=386827) [details]
> Test support library and one test for POST data

Please note that the patch contains two Mercurial renames that are not apparent
from the graphical diff representation in Bugzilla.

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 386827 [details] [diff] [review]
Test support library and one test for POST data [has renames]

I'm happy with this, with my comments (mostly nits) addressed.  A browser peer should probably look at this too.  I'd feel better if someone else looked it over as well because I probably missed something given it's size.

http://reviews.visophyte.org/r/386827/
on file: toolkit/content/tests/browser/Makefile.in line 46
> DIRS = common data

I'd prefer
DIRS = \
  commmon \
  data \
  $(NULL)


on file: toolkit/content/tests/browser/browser_save_post_data.js line 55
>   const baseUrl =

global nit: either use all caps for the variable name of constants, or prefix
it with k please.


on file: toolkit/content/tests/browser/browser_save_post_data.js line 59
>     // Display the outer page, and wait for it to be loaded. Loading the URI
>     //  doesn't generally raise any exception, but if an error page is
>     //  displayed, an exception will occur later during the test.

global nit: don't indent the next line of the comment.


on file: toolkit/content/tests/browser/browser_save_post_data.js line 68
>       // Submit the form in the outer page, then wait for both the outer
>       //  document and the inner frame to be loaded again

global nit: please end comments with punctuation.


on file: toolkit/content/tests/browser/browser_save_post_data.js line 75
>       } finally {

global nit: finally on a new line after closing brace


on file: toolkit/content/tests/browser/common/Makefile.in line 17
> # The Initial Developer of the Original Code is
> # Netscape Communications Corporation.
> # Portions created by the Initial Developer are Copyright (C) 1998
> # the Initial Developer. All Rights Reserved.

This doesn't seem right :p


on file: toolkit/content/tests/browser/common/_loadAll.js line 61
>     "toolkitFunctions.js"

nit: comma after last element so adding new items doesn't have to modify it
(this is legal JS!)


on file: toolkit/content/tests/browser/common/_loadAll.js line 67
>   for(var [, scriptName] in Iterator(scriptNames)) {

nit: space after for; use let


on file: toolkit/content/tests/browser/common/mockFilePicker.js line 89
>   show: function() {

name your function please!


on file: toolkit/content/tests/browser/common/mockFilePicker.js line 92
>     this.file.append(this.defaultString || "no_default_file_name");

Why not make this.defaultString equal to "no_default_file_name" on the
prototype?  Instances can still override it.


on file: toolkit/content/tests/browser/common/mockFilePicker.js line 106
> // This object can be used to activate the mock file picker

Can you do a better job of explaining how here?


on file: toolkit/content/tests/browser/common/mockFilePicker.js line 107
> var mockFilePickerRegisterer = new MockObjectRegisterer(
>                                "@mozilla.org/filepicker;1",
>                                MockFilePicker);

This looks confusing.  What about adding a new line after the =, and indent
for the constructor call?


on file: toolkit/content/tests/browser/common/mockObjects.js line 42
>  * @param aContractID        The ContractID of the component to replace, for
>  *                            example "@mozilla.org/filepicker;1".
>  * @param aReplacementCtor   The constructor function for the JavaScript object
>  *                            that will be created every time createInstance is
>  *                            called. This object must implement QueryInterface
>  *                            and provide the XPCOM interfaces required by the
>  *                            specified ContractID (for example
>  *                            Components.interfaces.nsIFilePicker).

global nit: I prefer to have
@param aVarName
       Description here
It gives you a lot more space to describe what's going on, and lets the
variable name stand out too.


on file: toolkit/content/tests/browser/common/mockObjects.js line 51
> function MockObjectRegisterer(aContractID, aReplacementCtor) {

global nit: braces start on the next line when declaring a function.


on file: toolkit/content/tests/browser/common/mockObjects.js line 64
>   register: function() {

anonymous function! (I'll stop commenting about these, but please fix all
instances)


on file: toolkit/content/tests/browser/common/mockTransferForContinuing.js line 49
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
>                                          Ci.nsIWebProgressListener2,
>                                          Ci.nsITransfer]),

nit:
  QueryInterface: XPCOMUtils.generateQI([
    Ci.nsIWebProgressListener,                     
Ci.nsIWebProgressListener2,
    Ci.nsITransfer,
  ]),


on file: toolkit/content/tests/browser/common/mockTransferForContinuing.js line 53
>   // --- nsIWebProgressListener interface functions ---

global-nit:
////////////[all the way up to 80 char boundary]
//// nsIWebProgressListener


on file: toolkit/content/tests/browser/common/mockTransferForContinuing.js line 57
>     if (aStatus != Cr.NS_OK)

I'm pretty sure we have other success codes, which is why we use NS_SUCCEEDED
in c++ instead of just checking for NS_OK.  bz would know if that matters
here.


on file: toolkit/content/tests/browser/common/mockTransferForContinuing.js line 66
>   onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress,
>    aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress) { },

nit: line up new icons with (


on file: toolkit/content/tests/browser/common/mockTransferForContinuing.js line 89
> // This object can be used to activate the mock transfer object

ditto on better explanation


on file: toolkit/content/tests/browser/common/toolkitFunctions.js line 62
> function callSaveWithMockObjects(aSaveFunction) {
>   mockFilePickerRegisterer.register();
>   try {
>     mockTransferForContinuingRegisterer.register();
>     try {
>       aSaveFunction();
>     } finally {
>       mockTransferForContinuingRegisterer.unregister();
>     }
>   } finally {
>     mockFilePickerRegisterer.unregister();
>   }

Not clear why the try-finallys are needed here.  I think it's so you always
call unregister, but aren't these tests asynchronous (although, I am pretty
sure you have made them sync with all the mock objects).  A comment here would
be helpful.


on file: toolkit/content/tests/browser/common/toolkitFunctions.js line 89
>   try {
>     var scrInputStream = Cc["@mozilla.org/scriptableinputstream;1"].
>                          createInstance(Ci.nsIScriptableInputStream);
>     scrInputStream.init(inputStream);
>     try {
>       // Assume that the file is much shorter than 1 MiB
>       return scrInputStream.read(1048576);
>     } finally {
>       scrInputStream.close();
>     }
>   } finally {
>     inputStream.close();
>   }

Comments about try-finally usage here would be helpful as well.


r=sdwilsh

Comment 18

[:Paolo Amadini]

Shawn, thank you *very much* for reviewing this! I'll be probably able to
get at this and address all the issues within less than a day.

(In reply to comment #17)
> A browser peer should probably look at this too.

Whom would you recommend?

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> Whom would you recommend?
Your best bet is vlad or gavin.

Comment 20

[:Paolo Amadini]

Attachment: Test support library and one test for POST data [has renames]

(In reply to comment #17)
> (From update of attachment 386827 [details] [diff] [review])
> global nit: please end comments with punctuation.

Fixed most multi-line comments. Would you like also short comments to end
with a period?

> on file: toolkit/content/tests/browser/common/Makefile.in line 17
> > # The Initial Developer of the Original Code is
> > # Netscape Communications Corporation.
> > # Portions created by the Initial Developer are Copyright (C) 1998
> > # the Initial Developer. All Rights Reserved.
> 
> This doesn't seem right :p

These makefiles are just one the copy of the other, with hardly any new
original work. That's why, after copying them, I don't update the license
boilerplate.

> on file: toolkit/content/tests/browser/common/mockFilePicker.js line 89
> >   show: function() {
> 
> name your function please!

I named all the non-empty functions, even though I don't really understand
why it's needed. It's not uncommon to find cases where, after some renaming,
the two names become unaligned.

> on file: toolkit/content/tests/browser/common/mockFilePicker.js line 92
> >     this.file.append(this.defaultString || "no_default_file_name");
> 
> Why not make this.defaultString equal to "no_default_file_name" on the
> prototype?  Instances can still override it.

Because it's valid for callers to set the property to an empty string.

Comment 21

[:Paolo Amadini]

I've still not addressed the following concern:

> on file: toolkit/content/tests/browser/common/mockTransferForContinuing.js line
> 57
> >     if (aStatus != Cr.NS_OK)
> 
> I'm pretty sure we have other success codes, which is why we use NS_SUCCEEDED
> in c++ instead of just checking for NS_OK.  bz would know if that matters
> here.

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #20)
> Fixed most multi-line comments. Would you like also short comments to end
> with a period?
Yes please.

> These makefiles are just one the copy of the other, with hardly any new
> original work. That's why, after copying them, I don't update the license
> boilerplate.
Then please use hg copy so most of it doesn't show up in the diff?  You are adding a new file, so you either own it, or you are copying it from somewhere.

> Because it's valid for callers to set the property to an empty string.
But why would they do that?  Couldn't they just not set it in that case?

Comment 23

[:Paolo Amadini]

(In reply to comment #22)
> (In reply to comment #20)
> > Fixed most multi-line comments. Would you like also short comments to end
> > with a period?
> Yes please.
> > These makefiles are just one the copy of the other
> Then please use hg copy

Sure. I'll include both changes in the next patch, together with the other
fixes that may be required after the next review.

> > > Why not make this.defaultString equal to "no_default_file_name" on
> > > the prototype?  Instances can still override it.
> > Because it's valid for callers to set the property to an empty string.
> But why would they do that? Couldn't they just not set it in that case?

They could, but they're not required to. For example, the property could
be set unconditionally based on the value of a variable or function
parameter. A file picker instance can also be reused multiple times.

Even though the nsIFilePicker interface doesn't mention this behavior
explicitly, a quick MXR search shows that it is relied upon at least in
in the JS implementation of nsIHelperAppLauncherDialog::promptForSaveToFile,
and maybe in other places.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> >     if (aStatus != Cr.NS_OK)
> 
> I'm pretty sure we have other success codes, which is why we use NS_SUCCEEDED
> in c++ instead of just checking for NS_OK.  bz would know if that matters
> here.

It's unlikely to matter in practice (non-NS_OK success nsresults are rarely used, and almost certainly aren't in this specific instance), but it doesn't hurt to use Components.isSuccessCode(aStatus) in these cases instead, which will handle them correctly.

Comment 25

[:Paolo Amadini]

Attachment: Test support library and one test for POST data [has renames]

In the idle time while waiting for the second-pass review, I've generated
this new patch with some of the previously requested minor changes.

Comment 26

[:Paolo Amadini]

I'm copying the relevant part from bug 420401 comment 0 (originally
bug 299372 comment 195) by Dan Mosedale:

---

For what it's worth, I think that in an ideal world, the
following things about save-link-as would get tested:

* traversing a link to an as-yet-authenticated server that requires auth
* traversing a link that's busted provides error notification
* traversing a link that doesn't return info quickly falls-back to the old
behavior
* traversing a link that does return it quickly uses it

---

Where could we place a wishlist for things to be tested by this new test
suite (if it makes sense)? I imagine something where individual tests can be
flagged as implemented or easily removed from the list. One-bug-per-test
seems a bit cumbersome to me.

Comment 27

[:Paolo Amadini]

Uhm, while looking at bug 299372, I noticed that the attached patch for
"Save Link As" is mostly "browser" code ("nsContextMenu.js"), that only
in some cases falls back to the "toolkit" code, while in other cases
uses the services of the "uriloader/exthandler".

I'm not entirely sure that the tests in comment 26 are relevant for a
"toolkit" test suite ("contentAreaUtils.js"). We would end up with tests
in one place testing code that's mainly located in another place.

The testing library itself could also be moved elsewhere in the future,
to be reused for example by new tests in the "browser" folder, but this
is out of the scope of this bug.

What do you think?

Comment 28

[:Paolo Amadini]

Gavin, are you fine with Shawn's review of the patch?

The patch has been thoroughfully reviewed; the last attachment addresses
also the last minor points, and is probably ready for checkin, needing
only an official Toolkit peer's approval.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #26)
> 
> Where could we place a wishlist for things to be tested by this new test
> suite (if it makes sense)? I imagine something where individual tests can be
> flagged as implemented or easily removed from the list. One-bug-per-test
> seems a bit cumbersome to me.

A wiki page?

Comment 30

[:Paolo Amadini]

(In reply to comment #29)
> (In reply to comment #26)
> > 
> > Where could we place a wishlist for things to be tested by this new test
> > suite (if it makes sense)? I imagine something where individual tests can be
> > flagged as implemented or easily removed from the list. One-bug-per-test
> > seems a bit cumbersome to me.
> 
> A wiki page?

What do you think of the following title?

https://wiki.mozilla.org/Browser-chrome_Test_Wishlist_for_Toolkit_and_Browser
(this page doesn't exist at present)

We may also create a meta bug and cross-link it with the wiki page.

Comment 31

[:Paolo Amadini]

Attachment: Test support library and one test for POST data [has renames] [Backout: Comment 35]

Updated the diff to take into account the patch for bug 501608.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 393423 [details] [diff] [review]
Test support library and one test for POST data [has renames]
[Backout: Comment 35]

Sorry to have blocked progress here, I was away on vacation for the past two weeks and didn't manage to get to this review (despite it being on my list of things to take a look at). I don't think I'll be able to review this in depth in the near future, but I also don't think that should stop this from landing given that shawn's already reviewed it.

Just looking at the high-level view of the changes, this refactoring makes sense.

I take it you've ensured that the test for 471962 still fails with the fix for that bug undone?

Comment 33

[:Paolo Amadini]

(In reply to comment #32)
> I take it you've ensured that the test for 471962 still fails with the fix for
> that bug undone?

I did that check on 2009-07-04 (comment 14), and I don't think the changes
that I made afterwards would affect that result.

> I don't think I'll be able to review this in depth
> in the near future, but I also don't think that should stop this from
> landing given that shawn's already reviewed it.

Thanks, that's what we hoped to hear :-) I remember you already told
something similar to me in IRC (this is test code after all), we just
needed to see it written on the bug. Sometimes the rules get in the way,
you know, but no problem for the delay in the end; now I'm setting the
checkin-needed keyword on the bug.

Comment 34

[Serge Gautherie (:sgautherie)]

Comment on attachment 393423 [details] [diff] [review]
Test support library and one test for POST data [has renames]
[Backout: Comment 35]


http://hg.mozilla.org/mozilla-central/rev/60899aeb80a7

Comment 35

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)
> (From update of attachment 393423 [details] [diff] [review])
> http://hg.mozilla.org/mozilla-central/rev/60899aeb80a7

http://hg.mozilla.org/mozilla-central/rev/23b65e557a17
http://hg.mozilla.org/mozilla-central/rev/6eb6a1720165
{
Backed out:
may cause test failures and/or leak
}

Comment 36

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> may cause test failures and/or leak

It did not cause the leak, but it seems to have caused browser_keyevents_during_autoscrolling.js (and/or others to fail.

Paolo, I don't know how you tested you patch ... Anyway, next time, have it
run through TryServer.

Comment 37

[:Paolo Amadini]

Attachment: Test support library and one test for POST data [has hg copies] [Checkin: Comment 42]

(In reply to comment #36)
> It did not cause the leak, but it seems to have caused
> browser_keyevents_during_autoscrolling.js (and/or others to fail.

OK, I spent the day on the bug. It took me no longer than 15 minutes
to find out the issue... the rest of the time was spent with rebuilding,
running tests and generating a new patch :-)

Long story short, the problem lies in the *order* the tests are run. It doesn't
matter whether the old test or the refactored one is run, the point is that
the previous patch renamed the test file, causing it to be executed after
"browser_keyevents_during_autoscrolling.js".

If I run "browser_keyevents_during_autoscrolling.js" first, or for what it
matters if I run it alone, it fails deterministically. If I run it after
the test for bug 471962, it succeeds.

Unfortunately, on my Windows XP machine, with the latest trunk (from 8 or 9
hours ago), rebuilt from scratch after deleting the objdir, neither the old
version (the one already on trunk) nor the refactored version of
"browser_bug471962.js" succeed. I have 40 other browser-chrome tests
failing in both cases.

> Paolo, I don't know how you tested you patch ...

In the wrong way. The last time I tested the patch, I used a --test-path to
run *only* the refactored test, and didn't check whether it worked well with
other tests, in particular with "browser_keyevents_during_autoscrolling.js",
that was added after I finished developing the first version of the patch.

> Anyway, next time, have it run through TryServer.

The TryServer is a great tool, excellent for cases like this, where it can be
used to save client computer / contributor time by verifying that an old
patch, already tested locally some time before, still applies on trunk.

Too bad I didn't know of its existence, since it's not mentioned on MDC in
the building and patching documentation. Is it accessible to non-committers?
The page I found on the Mozilla wiki says a Mozilla LDAP account is needed,
but I couldn't find out if and how I can get one. Or do you mean I should
ask someone else to do it for me when needed?

I attached a patch that is identical to the previous one, except for the
absence of the rename (the diffs look quite different because of this).
As I said, the test failed on my machine both with and without the patch.
Serge, since I haven't an LDAP account, can you run this attachment through
the TryServer for me to see if the test succeeds or if the patch causes
additional failures?

Comment 38

[:Paolo Amadini]

I don't have time to look into the real causes of the order dependency at
present, however I commented on bug 501608 where maybe Masayuki Nakano may
shed some light on this (I see there were other failures in the past,
quite similar to the ones I'm experiencing on my machine).

Meanwhile, running this patch through the TryServer may still be useful though.

Comment 39

[Serge Gautherie (:sgautherie)]

(In reply to comment #37)

> If I run "browser_keyevents_during_autoscrolling.js" first, or for what it
> matters if I run it alone, it fails deterministically. If I run it after
> the test for bug 471962, it succeeds.

Maybe there is something wrong with that test(s)...

> > Anyway, next time, have it run through TryServer.
> 
> Or do you mean I should ask someone else to do it for me when needed?

I guess so, but probably not until the issue(s) is fixed locally.

> Serge, [...] can you run this attachment through the TryServer for me

"Sorry", I'm not really interested in running Try (for someone else).


(In reply to comment #38)

> however I commented on bug 501608 where maybe Masayuki Nakano may
> shed some light on this (I see there were other failures in the past,
> quite similar to the ones I'm experiencing on my machine).

Indeed, Masayuki pointed me to some bug comment (I forgot) while on irc before I backed out.
I didn't really (had time to) look into that, then I simply proceeded.
But such dependency should be looked into and solved:
as in "may be browser_keyevents_during_autoscrolling.js should be disabled if causing trouble, and your test relanded"?!

Comment 40

[:Paolo Amadini]

Today I rebuilt the latest trunk locally, and now I can confirm that, using
attachment 395910 [details] [diff] [review], the test passes, even when run as part of the entire
browser-chrome suite. Probably the failure I experienced yesterday was caused
by another reason. In fact, that was that thing called "orange" on Tinderbox...
probably it just wasn't a good moment for running tests.

I still have some unrelated browser-chrome tests failing locally. Because of
this, to be sure that the patch is not causing trouble, it may be worth running
it through the TryServer. I'll ask in IRC if someone is willing to submit
it for me.

By the way, I was mistaken when I said TryServer is not mentioned on MDC: it
wasn't when I last read the "how to patch" documentation, but now that page
has improved a lot, and is much clearer. Just "try server" is spelled as two
words, that's why I didn't found it with my search.

If the TryServer build passes, I guess we can land the patch again, this time
without the rename, and I'll file a separate bug for dealing with the test
dependency problem.

Comment 41

[:Paolo Amadini]

TryServer build "try-65c04d1ddf47" started at 10.22.14 succeeded: this confirms
that the failures were caused the first time by the test reordering and the
second time by other failing tests executed before this (the "orange" state).

I filed bug 512100 to track progress with the test interdependency issue.

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 395910 [details] [diff] [review]
Test support library and one test for POST data [has hg copies]
[Checkin: Comment 42]


http://hg.mozilla.org/mozilla-central/rev/ecb8b1a47a62

Comment 43

[Arpad Borsos [:Swatinem]]

Comment on attachment 395910 [details] [diff] [review]
Test support library and one test for POST data [has hg copies]
[Checkin: Comment 42]

This patch is required for bug 295977 to apply/run without problems.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 395910 [details] [diff] [review]
Test support library and one test for POST data [has hg copies]
[Checkin: Comment 42]

I don't think bug 295977 will land on the branch, but assuming this is test-only (quick look suggests that it is), it can land on the branch without approval.