1286312 - Add mochitest option to run tests using https

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Doug Turner (:dougt)]

Attachment: 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

Comment 1

[Andrew Halberstadt [:ahal]]

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.

Comment 2

[Doug Turner (:dougt)]

Attachment: add_https_mochitest_option

Carrying forward r+.  Added "action": "store_true" to mochitest_options.py.

Comment 3

[Doug Turner (:dougt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18931a379bfb

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 8770230 [details] [diff] [review]
add_https_mochitest_option

looks like this does the wrong thing on chrome mochitests.

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Doug Turner (:dougt)]

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.

Comment 7

[Doug Turner (:dougt)]

Attachment: add_https_mochitest_option

Comment 8

[Doug Turner (:dougt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d522d5a9456

Comment 9

[(not currently active) Ted Mielczarek]

(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.

Comment 10

[Andrew Halberstadt [:ahal]]

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?

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Doug Turner (:dougt) from comment #7)
> Created attachment 8772186 [details] [diff] [review]
> add_https_mochitest_option

Missing reviewer?

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 14

[Andrew Halberstadt [:ahal]]

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.

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 17

[Andrew Halberstadt [:ahal]]

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.

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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 :)

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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

Comment 20

[James Graham [:jgraham]]

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.

Comment 21

[Andrew Halberstadt [:ahal]]

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.

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hello Malayaleecoder, may I understand if there's any update here? Thanks!

Comment 24

[Vishnu (: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?

Comment 25

[Vishnu (:malayaleecoder)]

Attachment: Bug1286312_wip.diff

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 28

[Andrew Halberstadt [:ahal]]

Sorry, this slipped. Jmaher/Vishnu, is this still being worked on or is it safe to unassign for now?

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

this is open for someone to take, I reset the assignee

Comment 30

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 31

[Paul Yang [:pyang] (away)]

Attachment: wip_patch

This patch roughly defines what I want to do.
Not sufficient to run though but will switch scheme dynamically by discussed.

Comment 32

[Andrew Halberstadt [:ahal]]

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.

Comment 33

[Paul Yang [:pyang] (away)]

Attachment: https_patch_wip

Based on off-line discussion, here is another wip patch which collects tests by schemes and run iteratively.

Comment 34

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/94950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94950/

Comment 35

[Paul Yang [:pyang] (away)]

Comment on attachment 8812094 [details] [diff] [review]
https_patch_wip

adjust review flag for obsolete patch

Comment 36

[Andrew Halberstadt [:ahal]]

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.

Comment 37

[Paul Yang [:pyang] (away)]

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 38

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/95360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95360/

Comment 39

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - cleanup with latest coding style

Review commit: https://reviewboard.mozilla.org/r/95362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95362/

Comment 40

[Andrew Halberstadt [:ahal]]

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.

Comment 41

[Andrew Halberstadt [:ahal]]

Comment on attachment 8814042 [details]
Bug 1286312 - cleanup with latest coding style

https://reviewboard.mozilla.org/r/95362/#review96498

Comment 42

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/96440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96440/

Comment 43

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - cleanup with latest coding style

Review commit: https://reviewboard.mozilla.org/r/96442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96442/

Comment 44

[Paul Yang [:pyang] (away)]

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.

Comment 45

[Paul Yang [:pyang] (away)]

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.

Comment 46

[Paul Yang [:pyang] (away)]

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

Carry on review+

Comment 47

[Paul Yang [:pyang] (away)]

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?

Comment 48

[Andrew Halberstadt [:ahal]]

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 49

[Andrew Halberstadt [:ahal]]

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.

Comment 50

[Paul Yang [:pyang] (away)]

(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 51

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/97650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97650/

Comment 52

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - cleanup with latest coding style

Review commit: https://reviewboard.mozilla.org/r/97652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97652/

Comment 53

[Paul Yang [:pyang] (away)]

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

carry review+; no change in this patch

Comment 54

[Paul Yang [:pyang] (away)]

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.

Comment 55

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 56

[Paul Yang [:pyang] (away)]

(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 57

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/97810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97810/

Comment 58

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - cleanup with latest coding style

Review commit: https://reviewboard.mozilla.org/r/97812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97812/

Comment 59

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 60

[Joel Maher ( :jmaher ) (UTC -8)]

the difference when running a single test file is that we don't load it in an iframe.

Comment 61

[Paul Yang [:pyang] (away)]

(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.

Comment 62

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 63

[Andrew Halberstadt [:ahal]]

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).

Comment 64

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/101510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101510/

Comment 65

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - cleanup with latest coding style

Review commit: https://reviewboard.mozilla.org/r/101512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101512/

Comment 66

[Paul Yang [:pyang] (away)]

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 67

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/101514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101514/

Comment 68

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - cleanup with latest coding style

Review commit: https://reviewboard.mozilla.org/r/101516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101516/

Comment 69

[Andrew Halberstadt [:ahal]]

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?

Comment 70

[Andrew Halberstadt [:ahal]]

Comment on attachment 8822640 [details]
Bug 1286312 - cleanup with latest coding style

https://reviewboard.mozilla.org/r/101516/#review102538

Comment 71

[Andreas Tolfsen ❲:ato❳]

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,)`.

Comment 72

[Paul Yang [:pyang] (away)]

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

[Andrew Halberstadt [:ahal]]

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 74

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - Add mochitest option to run tests using https

Review commit: https://reviewboard.mozilla.org/r/102918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102918/

Comment 75

[Paul Yang [:pyang] (away)]

Attachment: Bug 1286312 - cleanup with latest coding style

Review commit: https://reviewboard.mozilla.org/r/102920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102920/

Comment 76

[Andrew Halberstadt [:ahal]]

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 :).

Comment 77

[Andrew Halberstadt [:ahal]]

Comment on attachment 8824404 [details]
Bug 1286312 - cleanup with latest coding style

https://reviewboard.mozilla.org/r/102920/#review103478

Comment 78

[Andrew Halberstadt [:ahal]]

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.

Comment 79

[Andrew Halberstadt [:ahal]]

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 :)

Comment 80

[Paul Yang [:pyang] (away)]

try looks ok last Friday https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=66746765

I'll update and push another round today.

Comment 81

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

So what exactly the failures are?

Comment 82

[Andrew Halberstadt [:ahal]]

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

[Andrew Halberstadt [:ahal]]

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 :(

Comment 84

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/010b97cf49e1
https://hg.mozilla.org/mozilla-central/rev/65ff1adb85c0

Comment 86

[J.C. Jones [:jcj] (he/they)]

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?

Comment 87

[Andrew Halberstadt [:ahal]]

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?

Comment 88

[J.C. Jones [:jcj] (he/they)]

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.

Comment 89

[Paul Yang [:pyang] (away)]

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

Comment 90

[Matthew N. [:MattN]]

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

Comment 91

[Michelangelo De Simone [Offline] [:mds]]

(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.

Comment 92

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 93

[Matthew N. [:MattN]]

(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

Comment 94

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 95

[Julian Gaibler]

Hi!

This feature has, as far as I can see, never had any kind of documentation.
I'd love to do add it but can't tell if scheme=? can do anything else except enabling https for the test.

Comment 96

[Andrew Halberstadt [:ahal]]

Thanks, documentation would be great!

Glancing through the source, it looks like scheme can only be http or https. We should probably error out (here in an else clause) if we find anything else.