618283 - pagemod-test-helpers.js should be an official part of addon-kit and there should be configurable timeout

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Matěj Cepl]

Attachment: suggested patch

I believe it would be useful if an equivalent of packages/addon-kit/tests/pagemod-test-helpers.js was an official part of add-on sdk so that testPageMod could be require-d as any other function.

Also, I believe that for tests on real world webpages default timeout (or no timeout at all, documentation is not clear what is the default timeout) is too short, so it should be configurable when calling testPageMod in tests. Attached patch is one possible solution for that (patch is also available on gitorious as commit http://gitorious.org/addon-sdk/mass-close-duplicates/commit/ef90ca1f4e8a0bf5c7201199540a9782a711e4e5).

Comment 1

[Matěj Cepl]

Pull request https://github.com/mozilla/addon-sdk/pull/134

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 496799 [details] [diff] [review]
suggested patch

Testing against real live web pages seems dangerous, because the test depends on an external resource not under the harness's control (and subject to temporary unavailability or content changes).  The harness should set up a local web server that can serve pages to tests (including copies of live web pages).

But we don't have that right now.  And to some degree it's useful for addon developers to test against live pages, especially for page mods, since the tests will then let you know when the content of the page has changed in a way that breaks your page mod.

And this seems like a reasonable feature for tests that are using real live web pages, so r=myk!


>-  test.waitUntilDone();
>+  if (timeout !== undefined) {
>+	test.waitUntilDone(timeout);
>+  } else {
>+    test.waitUntilDone();
>+  }

Nit: per code conventions, don't cuddle elses.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #2)
> >-  test.waitUntilDone();
> >+  if (timeout !== undefined) {
> >+	test.waitUntilDone(timeout);
> >+  } else {
> >+    test.waitUntilDone();
> >+  }
> 
> Nit: per code conventions, don't cuddle elses.

Nit: actually this doesn't even need braces, since both blocks are one-liners.  And the first block uses a tab character rather than spaces for indentation.

Checked in with nits fixed!

https://github.com/mozilla/addon-sdk/commit/56a1b426c9d3c0f9645b3a092eca16ddb8622130

Comment 4

[Matěj Cepl]

(In reply to comment #2)
> Testing against real live web pages seems dangerous, because the test depends
> on an external resource not under the harness's control (and subject to
> temporary unavailability or content changes).  The harness should set up a
> local web server that can serve pages to tests (including copies of live web
> pages).

Yes, I am myself kind of the fence about testing live pages, and I have in the end tested against local dump of a page myself. OTOH, in the time of generated pages it might be necessary to use at least local testing HTTP server to test against, and in combination with latency of SQL database etc., we may well exceed the default timeout even locally. So, in the end I think it is worthwhile to have this option added.

> Nit: per code conventions, don't cuddle elses.

> Nit: actually this doesn't even need braces, since both blocks are
> one-liners. 

Two more reasons why jslint is considered harmful.

> Checked in with nits fixed!

Thanks a lot.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> Two more reasons why jslint is considered harmful.

Well, reasonable people can disagree, and the author of jslint and I (and Mozilla JS hackers generally) happen to disagree on this point. :-)

Comment 6

[Matěj Cepl]

(In reply to comment #5)
> (In reply to comment #4)
> > Two more reasons why jslint is considered harmful.
> 
> Well, reasonable people can disagree, and the author of jslint and I (and
> Mozilla JS hackers generally) happen to disagree on this point. :-)

Right, “considered harmful” is probably too strong. I am constantly frustrated by jslint mixing together awesome and useful features (looking for undeclared variables, missing parenthesis, and similar stuff) with random and sometimes quite peculiar personal opinions (only one variable declaration in the top of the function, this one). Fortunately, JavaScript strict mode seems to quite nicely preserving useful and omitting peculiar.