Closed Bug 892765 Opened 6 years ago Closed 6 years ago

free captivedetect tests from using a hardcoded server port


(Toolkit :: General, defect)

Not set





(Reporter: froydnj, Assigned: froydnj)




(3 files)

We need this for parallel xpcshell testing.
There's a lot of repeated code in these tests.  Moving it into a common file
will make future modifications much simpler.
Attachment #774320 - Flags: review?(schien)
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.
Attachment #774322 - Flags: review?(schien)
Now we can actually modify the global server to use a determined-at-runtime
port, instead of a hardcoded port 4444.
Attachment #774324 - Flags: review?(schien)
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.
Attachment #774320 - Flags: review?(schien) → review+
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
Attachment #774322 - Flags: review?(schien) → review+
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.
Attachment #774324 - Flags: review?(schien) → review+
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.