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

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mcepl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Created attachment 496799 [details] [diff] [review]
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).
(Reporter)

Updated

8 years ago
Attachment #496799 - Flags: review?(myk)
(Reporter)

Comment 1

8 years ago
Pull request https://github.com/mozilla/addon-sdk/pull/134
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.
Attachment #496799 - Flags: review?(myk) → review+
(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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
(Reporter)

Comment 4

8 years ago
(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.
(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. :-)
(Reporter)

Comment 6

8 years ago
(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.
You need to log in before you can comment on or make changes to this bug.