Add mochitest option to run tests using https

RESOLVED FIXED in Firefox 53

Status

Testing
Mochitest
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dougt, Assigned: pyang)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

Version 3
mozilla53
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [storage-v1])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 19 obsolete attachments)

59 bytes, text/x-review-board-request
ahal
: review+
Details | Review
59 bytes, text/x-review-board-request
ahal
: review+
Details | Review
(Reporter)

Description

2 years ago
Created attachment 8770213 [details] [diff] [review]
add_https_mochitest_option

This adds support for a mochitest option that runs mochitests 

1) Updates the build/pgo/certs/cert8.db and build/pgo/certs/key3.db

  We added mochi.test:443, then generated with the following instructions:
     https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest/Modifying_Mochitest_SSL_behavior


2) added new option --use-https
Attachment #8770213 - Flags: review?(ahalberstadt)
Comment on attachment 8770213 [details] [diff] [review]
add_https_mochitest_option

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

Lgtm, r+ once issue is fixed.

If we ever have tests that require either http or https, then we'll either need to break the https ones into a separate job that has this command line argument passed in, or we'll need to make https configurable in the mochitest.ini. The latter means we could mix http/https tests in the same job and the burden of knowing how the test is supposed to be run wouldn't fall on the developer, so that is what I would prefer. However it would require harness changes to support it. But this is a good intermediate step either way.

::: testing/mochitest/mochitest_options.py
@@ +113,5 @@
>                    "specify 'dist' to run tests against the distribution bundle's binary.",
>            }],
> +        [["--use-https"],
> +         {"dest": "useHttps",
> +          "default": False,

This needs to have "action": "store_true", otherwise argparse will expect a value here.
Attachment #8770213 - Flags: review?(ahalberstadt) → review+
(Reporter)

Comment 2

2 years ago
Created attachment 8770230 [details] [diff] [review]
add_https_mochitest_option

Carrying forward r+.  Added "action": "store_true" to mochitest_options.py.
Attachment #8770213 - Attachment is obsolete: true
Attachment #8770230 - Flags: review+
(Reporter)

Comment 4

2 years ago
Comment on attachment 8770230 [details] [diff] [review]
add_https_mochitest_option

looks like this does the wrong thing on chrome mochitests.
Attachment #8770230 - Flags: review+
Comment on attachment 8770230 [details] [diff] [review]
add_https_mochitest_option

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

::: testing/mochitest/mochitest_options.py
@@ +116,5 @@
> +         {"action": "store_true",
> +          "dest": "useHttps",
> +          "default": False,
> +          "help": "Run the mochitests using the https protocol instead of http.",
> +          }],

I'd rather us just turn this on by default, and then add (in a follow-up bug is fine) a way to run individual tests over HTTP. I don't see what value having this as an option buys us.
(Reporter)

Comment 6

2 years ago

So, turns out I learned a few things.  

* We already have a https server running during the mochitests.  It is at example.com:443.

* We only can have one https server at this point (see build/pgo/server-locations.txt)

* There are a number of tests that depend on the default https server Common Name to be "example.com"

So - maybe the thing to do is make the useHttps flag just force things to use example.com:443.
(Reporter)

Comment 7

2 years ago
Created attachment 8772186 [details] [diff] [review]
add_https_mochitest_option
Attachment #8770230 - Attachment is obsolete: true
Attachment #8772186 - Flags: review?
(In reply to Doug Turner (:dougt) from comment #6)
> 
> So, turns out I learned a few things.  
> 
> * We already have a https server running during the mochitests.  It is at
> example.com:443.
> 
> * We only can have one https server at this point (see
> build/pgo/server-locations.txt)

Er, that's not true, we serve all the domains listed with https over TLS for Mochitest.
So sounds like we should take an incarnation of the first patch that omits the argparse option and makes everything run over https except mochitest-chrome?
Yeah I think so. Also: this should block bug 1274315 (or that bug should be duped here) because loading the top-level page from https makes us unable to test certain web features.
See Also: → bug 1274315
(In reply to Doug Turner (:dougt) from comment #7)
> Created attachment 8772186 [details] [diff] [review]
> add_https_mochitest_option

Missing reviewer?
Flags: needinfo?(dougt)
Hi Doug and Andrew,
Bug 1267941 depends on this. Doug provided a patch for review without setting a reviewer, so I am wondering the status so far. Since you are the patch author and the most likely reviewer, may I understand your opinion about moving this forward? Thank you.
Flags: needinfo?(ahalberstadt)
So I think from ted and my discussion, we'd rather not accept the attached patch. Either a test should run over http or it should run over https. It doesn't make much sense for this to be a runtime argument.

The ideal patch would be to allow us to specify which protocol a test should use in the manifest. But since this feature currently doesn't exist, I guess we could take a patch similar to the one proposed here if it unblocks people though. Ted's proposal was to simply run *everything* over https for now. Though I can imagine that causing problems too.
Flags: needinfo?(dougt)
Flags: needinfo?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #14)
> So I think from ted and my discussion, we'd rather not accept the attached
> patch. Either a test should run over http or it should run over https. It
> doesn't make much sense for this to be a runtime argument.
> 
> The ideal patch would be to allow us to specify which protocol a test should
> use in the manifest. But since this feature currently doesn't exist, I guess
> we could take a patch similar to the one proposed here if it unblocks people
> though. 

That would be awesome!

> Ted's proposal was to simply run *everything* over https for now.
> Though I can imagine that causing problems too.

Updated

2 years ago
Blocks: 1268804
No longer blocks: 1267941
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #15)
> (In reply to Andrew Halberstadt [:ahal] from comment #14)
> > So I think from ted and my discussion, we'd rather not accept the attached
> > patch. Either a test should run over http or it should run over https. It
> > doesn't make much sense for this to be a runtime argument.
> > 
> > The ideal patch would be to allow us to specify which protocol a test should
> > use in the manifest. But since this feature currently doesn't exist, I guess
> > we could take a patch similar to the one proposed here if it unblocks people
> > though. 
> 
> That would be awesome!

Hi Andrew,
To clarify and follow your previous comment - so, would you be able to provide a patch for that? Thanks!

> 
> > Ted's proposal was to simply run *everything* over https for now.
> > Though I can imagine that causing problems too.
Flags: needinfo?(ahalberstadt)
Yes, I can provide a patch. But timeline is another matter, I'm kind of swamped at the moment and this isn't a trivial change. Let me chat with jgriffin tomorrow and we'll figure out where this falls priority wise or whether there are other resources on the team that can tackle it.

In the meantime, I always like to ask people.. Does this need to be a mochitest? Could you use either web-platform-tests instead? I asked jgraham, and wpt already supports running tests both over https and http, and that can be configured per test. Plus that way, you could potentially upstream your tests to other browsers if applicable (if not don't worry, there are also mozilla-specific tests). We always like to try to steer people away from mochitest when they are standing up new suites. We would love to slowly deprecate it.
Flags: needinfo?(ahalberstadt) → needinfo?(htsai)
(In reply to Andrew Halberstadt [:ahal] from comment #17)
> Yes, I can provide a patch. But timeline is another matter, I'm kind of
> swamped at the moment and this isn't a trivial change. Let me chat with
> jgriffin tomorrow and we'll figure out where this falls priority wise or
> whether there are other resources on the team that can tackle it.
> 

Thank you very much for the assistance. :)
If that helps, our target schedule for bug 1268804 is the end of this September.

> In the meantime, I always like to ask people.. Does this need to be a
> mochitest? Could you use either web-platform-tests instead? I asked jgraham,
> and wpt already supports running tests both over https and http, and that
> can be configured per test. Plus that way, you could potentially upstream
> your tests to other browsers if applicable (if not don't worry, there are
> also mozilla-specific tests). We always like to try to steer people away
> from mochitest when they are standing up new suites. We would love to slowly
> deprecate it.

Shawn would be able to answer this better than I :)
Flags: needinfo?(htsai) → needinfo?(shuang)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> (In reply to Andrew Halberstadt [:ahal] from comment #17)
> > In the meantime, I always like to ask people.. Does this need to be a
> > mochitest? Could you use either web-platform-tests instead? I asked jgraham,
> > and wpt already supports running tests both over https and http, and that
> > can be configured per test. Plus that way, you could potentially upstream
> > your tests to other browsers if applicable (if not don't worry, there are
> > also mozilla-specific tests). We always like to try to steer people away
> > from mochitest when they are standing up new suites. We would love to slowly
> > deprecate it.
> 
> Shawn would be able to answer this better than I :)
Storage API should cover test cases many different QuotaClients (ex:localStorage, indexed DB, cache API, application caches, notifications, etc).
We currently have Storage API mochitest test cases for Dom Cache and Indexeddb. We can try to write storage api wpt tests for indexeddb basically, but i'm not sure we can cover DOM cache test cases, we may need to use SpecialPowers.Services.qms (non-standard) that we cannot use in wpt, I guess that will limit our testing coverage.

http://searchfox.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_orphaned_body.html#27
http://searchfox.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_orphaned_body.html#64
Flags: needinfo?(shuang)
It's true that web-platform-tests don't have access to SpecialPowers. However we should endeavour to write all the tests that we can as wpt tests because then we can share them with other vendors and catch interoperability issues early rather than leaving it to the webcompat team to pick up the pieces later when sites rely on the specific behaviour of other browsers. So my recommendation would be to start by doing that and only use mochitests to fill in any gaps in the coverage that require non-web-exposed APIs to test.
Blocks: 1278370
It turns out there is another bug for this on file (bug 1278370) which one of our contributors was already investigating. So we'll be prioritizing mochitest for your use case, and I'll help malayaleecoder tackle this. In the meantime, please consider using WPT for however many tests you can for the reason jgraham mentioned above.

Malayaleecoder, the patch that dougt left here should be a good starting point. You'll more or less need to modify it to grab the base url information from the test manifest instead of the command line arguments. This means you'll need to somehow pass this information from the python side (runtests.py) to the javascript side. One approach would be to append '.https' to the test filename (and then strip that off again on the JS side).

Anyway it's a lot to take in, so please take some time to familiarize yourself with the code and ask myself or jmaher if you have any questions! Thanks for looking into this.
Assignee: doug.turner → malayaleecoder
Thank you Andrew and James for the suggestions. We are going to move toward the direction you mentioned - using wpt for as many tests as possible based on what it is now and only use mochitests to fill in the gaps. 

It's really great to have your help here as well, Malayaleecoder!
Hello Malayaleecoder, may I understand if there's any update here? Thanks!
Flags: needinfo?(malayaleecoder)
Hi,

Yes, me and :jmaher have been working on this for the last three weeks. When pushing to try there are a couple of errors that needs to be resolved which due to the hardcoded protocol and domains, present throughout the mochitests. This is taking up most of our time :(

Joel, do you have anything more to add on?
Flags: needinfo?(malayaleecoder) → needinfo?(jmaher)
as a note this patch is just a small change from what dougt put up originally.  This does change a lot of things, specifically assumptions made in test where we post messages from a child window to http://mochi.test:8888 (that won't work as that is not the parent window!).

you cam imagine all the hardcoded urls in tests- the work :malyaleecoder was doing was working through the more complicated tests in dom/security/test/mixedcontentblocker to see if we could make all of the tests work- so far that was not going smoothly.
Flags: needinfo?(jmaher)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> (In reply to Andrew Halberstadt [:ahal] from comment #17)
> > Yes, I can provide a patch. But timeline is another matter, I'm kind of
> > swamped at the moment and this isn't a trivial change. Let me chat with
> > jgriffin tomorrow and we'll figure out where this falls priority wise or
> > whether there are other resources on the team that can tackle it.
> > 
> 
> Thank you very much for the assistance. :)
> If that helps, our target schedule for bug 1268804 is the end of this
> September.

We've revised our test case plan for bug 1268804 that this one isn't an urgent blocker for that then.
No longer blocks: 1268804
Sorry, this slipped. Jmaher/Vishnu, is this still being worked on or is it safe to unassign for now?
Flags: needinfo?(malayaleecoder)
Flags: needinfo?(jmaher)
this is open for someone to take, I reset the assignee
Assignee: malayaleecoder → nobody
Flags: needinfo?(malayaleecoder)
Flags: needinfo?(jmaher)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #27)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> > (In reply to Andrew Halberstadt [:ahal] from comment #17)
> > > Yes, I can provide a patch. But timeline is another matter, I'm kind of
> > > swamped at the moment and this isn't a trivial change. Let me chat with
> > > jgriffin tomorrow and we'll figure out where this falls priority wise or
> > > whether there are other resources on the team that can tackle it.
> > > 
> > 
> > Thank you very much for the assistance. :)
> > If that helps, our target schedule for bug 1268804 is the end of this
> > September.
> 
> We've revised our test case plan for bug 1268804 that this one isn't an
> urgent blocker for that then.

Sorry, I'm probably over-optimistic. Even I converted my storage mochitest cases to wpt, it's not enough.
#1 dom/tests/mochitest/general/test_interfaces.html
#2 dom/workers/test/test_navigator.html
#3 dom/workers/test/test_worker_interfaces.html
There are some other tests which verify interfaces correctly exposed, so with SecureContext annotation, these tests could still fail.
Assignee: nobody → pyang
Status: NEW → ASSIGNED
(Assignee)

Comment 31

a year ago
Created attachment 8808975 [details] [diff] [review]
wip_patch

This patch roughly defines what I want to do.
Not sufficient to run though but will switch scheme dynamically by discussed.
Attachment #8808975 - Flags: feedback?(ahalberstadt)
Comment on attachment 8808975 [details] [diff] [review]
wip_patch

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

Interesting, I never realized the full test object was dumped to the json manifest, but it is.. so I think this is a good approach!

::: testing/mochitest/manifestLibrary.js
@@ +29,5 @@
>        links[name] = {'test': {'url': name, 'expected': obj['expected']}};
>      } else {
>        name = params.testPrefix + path;
> +      if ("scheme" in obj && obj['scheme'] == "https"){
> +        name = "https://example.com" + name;

I think you still need to use params.testPrefix here. You may need to strip the scheme off it first though.

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +407,5 @@
>          !TestRunner._haltTests)
>      {
>          var url = TestRunner.getNextUrl();
> +        SpecialPowers.pushPrefEnv({'set': [["security.mixed_content.block_active_content", false],
> +                                           ["security.mixed_content.block_display_content", false]]});

For now try setting these in this file:
https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js

If that works we can look into setting these dynamically.
Attachment #8808975 - Flags: feedback?(ahalberstadt) → feedback+
Attachment #8793078 - Attachment is obsolete: true
Attachment #8793078 - Flags: feedback?
(Assignee)

Comment 33

a year ago
Created attachment 8812094 [details] [diff] [review]
https_patch_wip

Based on off-line discussion, here is another wip patch which collects tests by schemes and run iteratively.
Attachment #8808975 - Attachment is obsolete: true
Attachment #8812094 - Flags: feedback?(ahalberstadt)
(Assignee)

Updated

a year ago
Blocks: 1268804
Comment hidden (mozreview-request)
(Assignee)

Comment 35

a year ago
Comment on attachment 8812094 [details] [diff] [review]
https_patch_wip

adjust review flag for obsolete patch
Attachment #8812094 - Flags: feedback?(ahalberstadt)

Comment 36

a year ago
mozreview-review
Comment on attachment 8813518 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/94950/#review95350

Thanks, this is starting to look good! Does this appear to work on try if you add 'scheme=https' to some tests?

::: dom/indexedDB/test/mochitest.ini
(Diff revision 1)
>  [test_setVersion.html]
>  [test_setVersion_abort.html]
>  [test_setVersion_events.html]
>  [test_setVersion_exclusion.html]
>  [test_setVersion_throw.html]
> -[test_storage_manager_estimate.html]

I assume you meant to add scheme=https here instead of deleting this test?

::: testing/mochitest/runtests.py:54
(Diff revision 1)
>  )
>  
>  try:
>      from marionette import Marionette
>      from marionette_driver.addons import Addons
> -except ImportError, e:
> +except ImportError as e:

It would be nice if these cleanups were in a separate commit.

::: testing/mochitest/runtests.py:1016
(Diff revision 1)
> -    def buildTestURL(self, options):
> +    def buildTestURL(self, options, scheme='http'):
> +        if scheme == 'https':
> +            testHost = "https://example.com"
> +        else:
> -        testHost = "http://mochi.test:8888"
> +            testHost = "http://mochi.test:8888"

Please try adding https://mochi.test:8888 with the 'privileged' capability to build/pgo/server-locations.txt and see of that works as the server url.

::: testing/mochitest/runtests.py:2383
(Diff revision 1)
>                  marionette_args['host'] = host
>                  marionette_args['port'] = int(port)
>  
> +            # testsToFilter parameter is used to filter out the test list that
> +            # is sent to buildTestPath
> +            for (scheme, tests) in self.buildTestPath(options, testsToFilter):

I think we should rename the 'buildTestPath method, as it isn't really doing that anymore.

::: testing/mochitest/tests/SimpleTest/setup.js:163
(Diff revision 1)
>  RunSet.runall = function(e) {
>    // Filter tests to include|exclude tests based on data in params.filter.
>    // This allows for including or excluding tests from the gTestList
>    // TODO Only used by ipc tests, remove once those are implemented sanely
>    if (params.testManifest) {
> -    getTestManifest("http://mochi.test:8888/" + params.testManifest, params, function(filter) { gTestList = filterTests(filter, gTestList, params.runOnly); RunSet.runtests(); });
> +    getTestManifest(getTestManifest(params.testManifest), params, function(filter) { gTestList = filterTests(filter, gTestList, params.runOnly); RunSet.runtests(); });

This should be getTestManifestURL I think.
Attachment #8813518 - Flags: review?(ahalberstadt)
(Assignee)

Comment 37

a year ago
mozreview-review-reply
Comment on attachment 8813518 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/94950/#review95350

> I assume you meant to add scheme=https here instead of deleting this test?

No, I applied an old patch from developer and didn't cleanup it.  Thanks!

> It would be nice if these cleanups were in a separate commit.

Will put all those in another commit.

> I think we should rename the 'buildTestPath method, as it isn't really doing that anymore.

Agree.  It runs manitest filters and then writes test attributes to json file.
How about something like 'prepare_manifest_file'?

> This should be getTestManifestURL I think.

Thanks, will fix in next revision
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8813518 - Attachment is obsolete: true

Comment 40

a year ago
mozreview-review
Comment on attachment 8814041 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/95360/#review96496

Thanks this is looking good, but because of the first issue we won't be able to land it. It seems like the testHost is not being set properly between restarts.

::: testing/mochitest/runtests.py:1016
(Diff revision 1)
> -    def buildTestURL(self, options):
> +    def buildTestURL(self, options, scheme='http'):
> +        if scheme == 'https':
> +            testHost = "https://mocchi.test:8888"
> +        else:
> -        testHost = "http://mochi.test:8888"
> +            testHost = "http://mochi.test:8888"

This doens't seem to work when running both http and https tests. Here's the STR:

1. Add scheme = https to only one test in caps/tests/mochitest/mochitest.ini
2. Run: ./mach mochitest -f plain caps

We first correctly run all the http tests, but when we restart the browser to run the https ones, then the testHost is still set to http://mochi.test:8888 when it should be https://example.com:433.

We'll need to make sure we recalculate the testHost in between browser restarts.

::: testing/mochitest/runtests.py:1018
(Diff revision 1)
>              self.testRoot = self.TEST_PATH
>          self.testRootAbs = os.path.join(SCRIPT_DIR, self.testRoot)
>  
> -    def buildTestURL(self, options):
> +    def buildTestURL(self, options, scheme='http'):
> +        if scheme == 'https':
> +            testHost = "https://mocchi.test:8888"

There is a typo here, though I see what you mean, that even fixing this and switching to 443 doesn't work. I guess let's just use https://example.com:443 for now.
Attachment #8814041 - Flags: review-

Comment 41

a year ago
mozreview-review
Comment on attachment 8814042 [details]
Bug 1286312 - cleanup with latest coding style

https://reviewboard.mozilla.org/r/95362/#review96498
Attachment #8814042 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8814041 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8814042 - Attachment is obsolete: true
(Assignee)

Comment 44

a year ago
Sorry for the typo but I did correct it and test however it didn't work as expected.
Updated and I'll trigger another run for this.
(Assignee)

Comment 45

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0edc40b6fd9adeb850c491b535f5dc52a7b0897&selectedJob=32150819
Here's try result, some jobs failed but turned to success after re-trigger.
(Assignee)

Comment 46

a year ago
Comment on attachment 8815561 [details]
Bug 1286312 - cleanup with latest coding style

Carry on review+
Attachment #8815561 - Flags: review+
(Assignee)

Comment 47

a year ago
Comment on attachment 8815560 [details]
Bug 1286312 - Add mochitest option to run tests using https

Change:
- go back to https://example.com:443
- minor modification for style

Actually we didn't come to conclusion for the name of "buildTestPath", for instance 'prepare_manifest_file'.  Andrew, do you have any suggestion for this?
Attachment #8815560 - Flags: feedback?(ahalberstadt)
Attachment #8772186 - Attachment is obsolete: true
Attachment #8772186 - Flags: review?
Attachment #8812094 - Attachment is obsolete: true

Comment 48

a year ago
mozreview-review
Comment on attachment 8815560 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/96440/#review97588

::: testing/mochitest/runtests.py:1042
(Diff revision 1)
>              testURL = "about:blank"
>          if options.nested_oop:
>              testURL = "/".join([testHost, self.NESTED_OOP_TEST_PATH])
>          return testURL
>  
>      def buildTestPath(self, options, testsToFilter=None, disabled=True):

Maybe this could be called "getTestsByScheme" ?
Comment on attachment 8815560 [details]
Bug 1286312 - Add mochitest option to run tests using https

Thanks, looks good! Put name suggestion in the review comment.
Attachment #8815560 - Flags: feedback?(ahalberstadt) → feedback+
(Assignee)

Comment 50

a year ago
(In reply to Andrew Halberstadt [:ahal] from comment #48)
> Comment on attachment 8815560 [details]
> Bug 1286312 - Add mochitest option to run tests using https
> 
> https://reviewboard.mozilla.org/r/96440/#review97588
> 
> ::: testing/mochitest/runtests.py:1042
> (Diff revision 1)
> >              testURL = "about:blank"
> >          if options.nested_oop:
> >              testURL = "/".join([testHost, self.NESTED_OOP_TEST_PATH])
> >          return testURL
> >  
> >      def buildTestPath(self, options, testsToFilter=None, disabled=True):
> 
> Maybe this could be called "getTestsByScheme" ?

I think this fits its usage.  Update patch with name change.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8815560 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8815561 - Attachment is obsolete: true
(Assignee)

Comment 53

a year ago
Comment on attachment 8817334 [details]
Bug 1286312 - cleanup with latest coding style

carry review+; no change in this patch
Attachment #8817334 - Flags: review+
(Assignee)

Comment 54

a year ago
Comment on attachment 8817333 [details]
Bug 1286312 - Add mochitest option to run tests using https

- function buildTestPath -> getTestsByScheme
have tested some dom test suites by applying scheme=https, looks ok.
Attachment #8817333 - Flags: review?(ahalberstadt)
(In reply to Paul Yang [:pyang] from comment #54)
> Comment on attachment 8817333 [details]
> Bug 1286312 - Add mochitest option to run tests using https
> 
> - function buildTestPath -> getTestsByScheme
> have tested some dom test suites by applying scheme=https, looks ok.

I think you forgot to rename buildTestPath all the places.
(Assignee)

Comment 56

a year ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #55)
> (In reply to Paul Yang [:pyang] from comment #54)
> > Comment on attachment 8817333 [details]
> > Bug 1286312 - Add mochitest option to run tests using https
> > 
> > - function buildTestPath -> getTestsByScheme
> > have tested some dom test suites by applying scheme=https, looks ok.
> 
> I think you forgot to rename buildTestPath all the places.

Thanks, I've pushed another try run and update the patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8817333 - Attachment is obsolete: true
Attachment #8817333 - Flags: review?(ahalberstadt)
(Assignee)

Updated

a year ago
Attachment #8817334 - Attachment is obsolete: true
Interesting I found that doing "mach mochitest dom/workers/test" and "mach mochitest dom/workers/test/test_navigator.html" may have different results locally.

So we don't need to run tests on try-server to verify test results, just specify folder location (dom/workers/test) locally, url can't be replaced as https://example.com correctly.
the difference when running a single test file is that we don't load it in an iframe.
(Assignee)

Comment 61

a year ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #59)
> Interesting I found that doing "mach mochitest dom/workers/test" and "mach
> mochitest dom/workers/test/test_navigator.html" may have different results
> locally.
> 
> So we don't need to run tests on try-server to verify test results, just
> specify folder location (dom/workers/test) locally, url can't be replaced as
> https://example.com correctly.

We passed start_script_args with testURL by
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1940

However it didn't notice we have 2 run by different schemes now, and so second run for https simply append its URL while outplut handler takes last run's argument.
(In reply to Paul Yang [:pyang] from comment #61)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #59)
> > Interesting I found that doing "mach mochitest dom/workers/test" and "mach
> > mochitest dom/workers/test/test_navigator.html" may have different results
> > locally.
> > 
> > So we don't need to run tests on try-server to verify test results, just
> > specify folder location (dom/workers/test) locally, url can't be replaced as
> > https://example.com correctly.
> 
> We passed start_script_args with testURL by
> https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.
> py#1940
> 
> However it didn't notice we have 2 run by different schemes now, and so
> second run for https simply append its URL while outplut handler takes last
> run's argument.

Any update? Is it working to clean the arguments?
Flags: needinfo?(pyang)
Paul will be out for several weeks now. I'll take a look at finishing this up when I get a chance (won't be until after the holidays though).
Flags: needinfo?(pyang)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8817578 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8817579 - Attachment is obsolete: true
(Assignee)

Comment 66

a year ago
Mochitest(runtests.py) sents arguments by assigning self.start_script_args to specify flavor and testUrl(for master web page), and in js side it takes first two arguments of self.start_script_args.

By chunking we might have several runs, and each of them appends variables to self.start_script_args.  It might have potential error while it ran many times, self.start_script_args will become
[flavor1, testUrl1, flavor2, testUrl2, flavor3, testUrl3, flavor4, testUrl4, flavor5, testUrl5, ....]

We change to use keyword arguments and only build its arguments before executing start_script.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8822631 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8822632 - Attachment is obsolete: true

Comment 69

a year ago
mozreview-review
Comment on attachment 8822639 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/101514/#review102534

::: testing/mochitest/runtests.py:1060
(Diff revision 1)
>              if testsToFilter and (test['path'] not in testsToFilter):
>                  continue
>              paths.append(test)
>  
> +        # Generate test by schemes
> +        for (scheme, grouped_tests) in self.groupTestsByScheme(paths).items():

nit: first set of brackets are unnecessary.

::: testing/mochitest/runtests.py:1572
(Diff revision 1)
> +            if 'flavor' in self.start_script_kwargs:
> +                start_script_args.append(self.start_script_kwargs['flavor'])
> +            if 'testUrl' in self.start_script_kwargs:
> +                start_script_args.append(self.start_script_kwargs['testUrl'])
> +            return self.marionette.execute_script(script, script_args=start_script_args)

I don't really see the point of making this a dict, then converting it back to a list here. Can we just leave it like it was?
Attachment #8822640 - Flags: review?(ahalberstadt)

Comment 70

a year ago
mozreview-review
Comment on attachment 8822640 [details]
Bug 1286312 - cleanup with latest coding style

https://reviewboard.mozilla.org/r/101516/#review102538
Attachment #8822640 - Flags: review?(ahalberstadt) → review+

Comment 71

a year ago
mozreview-review-reply
Comment on attachment 8822639 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/101514/#review102534

> I don't really see the point of making this a dict, then converting it back to a list here. Can we just leave it like it was?

I want to point out that `script_args` takes both tuples and lists, but please use tuples if you can since this is most Pythonic.  Of course you can pass a dict as the only argument of the argument list using `script_args=(my_dict,)`.
(Assignee)

Comment 72

a year ago
mozreview-review-reply
Comment on attachment 8822639 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/101514/#review102534

> I want to point out that `script_args` takes both tuples and lists, but please use tuples if you can since this is most Pythonic.  Of course you can pass a dict as the only argument of the argument list using `script_args=(my_dict,)`.

I think to use keyword arguments makes sense here because it looks more readable; and original code simply appends arguments so I just followed. Per ato mentioned marionette.execute_script accepts dict I can change to use tuple + dict.

Comment 73

a year ago
mozreview-review-reply
Comment on attachment 8822639 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/101514/#review102534

> I think to use keyword arguments makes sense here because it looks more readable; and original code simply appends arguments so I just followed. Per ato mentioned marionette.execute_script accepts dict I can change to use tuple + dict.

Yeah, I like that idea too, let's use the dict.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8822639 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8822640 - Attachment is obsolete: true
Please add r?ahal to your commit message so mozreview properly flags me for review (or manually flag me in mozreview). Otherwise I will not realize I'm supposed to look at this, I get lots of bugmail, so it's easy to miss :).
Attachment #8824403 - Flags: review?(ahalberstadt)
Attachment #8824404 - Flags: review?(ahalberstadt)

Comment 77

a year ago
mozreview-review
Comment on attachment 8824404 [details]
Bug 1286312 - cleanup with latest coding style

https://reviewboard.mozilla.org/r/102920/#review103478
Attachment #8824404 - Flags: review?(ahalberstadt) → review+

Updated

a year ago
Blocks: 1329802

Comment 78

a year ago
mozreview-review
Comment on attachment 8824403 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/102918/#review104152

Thanks, this looks good! I want to do a bit more testing before this lands though. If all looks good, I'll autoland this on your behalf later today.
Attachment #8824403 - Flags: review?(ahalberstadt) → review+

Comment 79

a year ago
mozreview-review
Comment on attachment 8824403 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/102918/#review104274

Actually, on testing further, this seems to break mochitests for me. Please test that it works when uploading to review :)
Attachment #8824403 - Flags: review+ → review-
(Assignee)

Comment 80

a year ago
try looks ok last Friday https://treeherder.mozilla.org/#/jobs?repo=try&author=pyang@mozilla.com&selectedJob=66746765

I'll update and push another round today.
So what exactly the failures are?
Just try running mochitest locally with this patch and it doesn't work, no need to use try. The failure looks related to using the dictionary as the marionette argument. It's important we test both on try and locally, because there are code paths that only get hit when running through mach (though in this case I think both would fail).

Comment 83

a year ago
mozreview-review
Comment on attachment 8824403 [details]
Bug 1286312 - Add mochitest option to run tests using https

https://reviewboard.mozilla.org/r/102918/#review104658

Oh wait sorry, I think there's something else going on. I was using:
./mach mochitest caps

to test that, but trying other directories works. I just checked and the caps directory seems to be broken, even without this patch. So I guess I was seeing a different problem. Sorry for the confusion :(
Attachment #8824403 - Flags: review- → review+

Comment 84

a year ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/010b97cf49e1
Add mochitest option to run tests using https r=ahal
https://hg.mozilla.org/integration/autoland/rev/65ff1adb85c0
cleanup with latest coding style r=ahal

Comment 85

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/010b97cf49e1
https://hg.mozilla.org/mozilla-central/rev/65ff1adb85c0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 86

a year ago
Running locally with a directory that has half the tests marked "scheme: https" in mochitest.ini, I'm pretty consistently getting

TEST-UNEXPECTED-FAIL: setup.js | error loading https://example.com/tests.json,tests.json

and having all the HTTPS tests fail to execute. The HTTP tests do fine.

That error happens on invocations of:

mach mochitest dom/u2f/tests
mach mochitest dom/u2f
cd dom/u2f/tests ; mach mochitest .
cd dom/u2f/tests ; mach mochitest test_*

Individual runs with (updated to require https) tests like:
cd dom/u2f/tests ; mach mochitest test_register_sign.html

...behave fine, so long as there's not a mix of http/https tests in one directory.

This also just happened to me on try in this job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9ac0ba89ed324b0dc2e6e2d19f3fc57474fea8&selectedJob=68466425

That's from code in this review: https://reviewboard.mozilla.org/r/103600/diff/3#index_header

Perhaps I'm invoking it wrong? Or should I split http/https tests by directory?
A mix of http/https in the same manifest should work,if it doesn't, then there's a bug in the patch somewhere. Paul, mind taking another look?
Flags: needinfo?(pyang)

Comment 88

a year ago
It appears to me that to get this error message

TEST-UNEXPECTED-FAIL: setup.js | error loading https://example.com/tests.json,tests.json

then tests/SimpleTest/setup.js getTestManifestURL() has to be passed the string "tests.json,tests.json", which seems to mean the manifest file's got that within it.
(Assignee)

Comment 89

a year ago
We still need to cleanup self.urlOpts before running for https scheme; the options append at each run and cause this.  Sorry for that.

quick patch I tested in my repo, and it works fine.

--- a/testing/mochitest/runtests.py
+++ b/testing/mochitest/runtests.py
@@ -2073,6 +2073,7 @@ toolbar#nav-bar {
             # cleanup
             if os.path.exists(processLog):
                 os.remove(processLog)
+            self.urlOpts = []
 
         return status
Flags: needinfo?(pyang)
(Assignee)

Updated

a year ago
Depends on: 1330867
Depends on: 1332573

Updated

a year ago
Blocks: 1274315

Updated

a year ago
Blocks: 1333140
When this is working properly it would be great if an email was sent to firefox-dev and dev-platform.
Keywords: dev-doc-needed
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #90)

> When this is working properly it would be great if an email was sent to
> firefox-dev and dev-platform.

Agreed: this is a very nice improvement.
FWIW this works beautifully in my case and made my life so much easier for a geo patch I'm working on.
Hi Paul,
If I run "mach mochitest -f browser dom/quota/test" with bug 1268804, I added scheme=https in browser.ini, that doesn't seem to work. Is there something I missed here? I thought browser chrome test still part of mochitest? If it's not part of mochitest config file, how can I add it?
Flags: needinfo?(pyang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #92)
> Hi Paul,
> If I run "mach mochitest -f browser dom/quota/test" with bug 1268804, I
> added scheme=https in browser.ini, that doesn't seem to work. Is there
> something I missed here? I thought browser chrome test still part of
> mochitest? If it's not part of mochitest config file, how can I add it?

This option doesn't make sense for mochitest-browser-chrome since the browser_*.js files aren't loaded over HTTP. Your browser_*.js have been able to open tabs pointing to https local test domains for a long time e.g. https://example.com. See https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests#Support-files
Flags: needinfo?(pyang)
(In reply to Matthew N. [:MattN] (Meetings In Taipei) from comment #93)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #92)
> > Hi Paul,
> > If I run "mach mochitest -f browser dom/quota/test" with bug 1268804, I
> > added scheme=https in browser.ini, that doesn't seem to work. Is there
> > something I missed here? I thought browser chrome test still part of
> > mochitest? If it's not part of mochitest config file, how can I add it?
> 
> This option doesn't make sense for mochitest-browser-chrome since the
> browser_*.js files aren't loaded over HTTP. Your browser_*.js have been able
> to open tabs pointing to https local test domains for a long time e.g.
> https://example.com. See
> https://developer.mozilla.org/en-US/docs/Mozilla/
> Browser_chrome_tests#Support-files

You're correct. I indeed need to do some self-reviewing before submitting questions. Thanks for the hint.
Whiteboard: [storage-v1]
You need to log in before you can comment on or make changes to this bug.