892765 - free captivedetect tests from using a hardcoded server port

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Nathan Froyd [:froydnj]]

We need this for parallel xpcshell testing.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move common captivedetect test code into head_setprefs.js

There's a lot of repeated code in these tests.  Moving it into a common file
will make future modifications much simpler.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - rewrite captivedetect tests in terms of run_captivedetect_test

Now that we have common code, let's refactor the running of the actual tests to
use common code as well.  No functionality changes intended.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - run captivedetect test http server on OS-determined port

Now we can actually modify the global server to use a determined-at-runtime
port, instead of a hardcoded port 4444.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 774320 [details] [diff] [review]
part 1 - move common captivedetect test code into head_setprefs.js

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

r=me with my comment addressed.

::: toolkit/components/captivedetect/test/unit/head_setprefs.js
@@ +20,5 @@
>  const kPrefsCanonicalURL = 'captivedetect.canonicalURL';
>  const kPrefsCanonicalContent = 'captivedetect.canonicalContent';
>  const kPrefsMaxWaitingTime = 'captivedetect.maxWaitingTime';
>  const kPrefsPollingTime = 'captivedetect.pollingTime';
> +const kInterfaceName = 'wifi';

I prefer to leave kInterfaceName in the each test file, so that you don't need to open another file to understand the semantic of this variable.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 774322 [details] [diff] [review]
part 2 - rewrite captivedetect tests in terms of run_captivedetect_test

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

LGTM, r=me with the comment addressed.

::: toolkit/components/captivedetect/test/unit/head_setprefs.js
@@ +43,5 @@
> +
> +  setupPrefs();
> +
> +  fakeUIResponse();
> + 

nit: extra space

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 774324 [details] [diff] [review]
part 3 - run captivedetect test http server on OS-determined port

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

LGTM, please remember to put the try server result in this bug.

Comment 7

[Nathan Froyd [:froydnj]]

https://tbpl.mozilla.org/?tree=Try&rev=141ae133e1aa

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b8c0b71c556
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8dfaa3d620fb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/332c413c55e8

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b8c0b71c556
https://hg.mozilla.org/mozilla-central/rev/8dfaa3d620fb
https://hg.mozilla.org/mozilla-central/rev/332c413c55e8