754847 - Replace local httpd.js web server with wptserve

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

As talked with Clint we also want to get rid of the old JS implementation of httpd and replace it with the new mozhttpd server.

It was fine by Clint to have it for Mozmill 2.0, but not sure if it should be a blocker or not. So lets discuss it again.

Comment 1

[cmtalbert]

Not a blocker, but a really really really nice to have.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Adding blocking mozmill 2.0 because it blocks 767332 which is a blocker for mozmill 2.0.

I will check that in the next days.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Given that my patch on bug 767332 fixes the problem we are currently facing, I don't see a reason why we have to swap the current httpd.js server with mozhttpd for Mozmill 2.0. Putting back on the request list.

Comment 4

[Andrei Eftimie]

Taking this. Its part of our current quarterly goals.

Comment 5

[Andrei Eftimie]

Attachment: WIP 1

WIP patch. 

Still lots of stuff to do:
1) The root has to be given manually with --base-url, not clear whether we'll keep this or implement an algorithm that will try to guess (on the current mozmill-tests repo) the /data/ folder. We can do this easily for mutt and mozmill-automation.

2) If we shutdown firefox (controller.stopApplication) we lose the server. Not sure yet what the best venue here will be. (Current code tries to remember the state of the server, but it doesn't work correctly. A fail-proof mechanism determining if the server is running or not would probably be best. mozhttpd doesn't offer any direct way of checking this

3) Need a mutt test

And others changes will be needed.
a) In mozmill-tests we'll need to remove all addHttpResource calls
b) mozmill-automation will need support
4)

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #5)
> 1) The root has to be given manually with --base-url, not clear whether
> we'll keep this or implement an algorithm that will try to guess (on the
> current mozmill-tests repo) the /data/ folder. We can do this easily for
> mutt and mozmill-automation.

I would have expected a follow-up comment for that here given that I answered this in the ask and expert meeting. So regarding that situation, we may want to stick that in the manifest. But first I would suggest we check how Marionette is handling that.

> 2) If we shutdown firefox (controller.stopApplication) we lose the server.
> Not sure yet what the best venue here will be. (Current code tries to
> remember the state of the server, but it doesn't work correctly. A
> fail-proof mechanism determining if the server is running or not would
> probably be best. mozhttpd doesn't offer any direct way of checking this

Why do we loose the server? It's running completely separated from the Firefox process. So nothing should bring down the server as long as Mozmill is running on the Python side.

> a) In mozmill-tests we'll need to remove all addHttpResource calls

I would suggest we try to stay backward compatible, and keep that method. It should simply return the base url. We can remove it with the next major version of Mozmill.

> b) mozmill-automation will need support

Not if we can handle all that via the manifest files.

> 4)

? :)

Comment 7

[Andrei Eftimie]

Attachment: WIP 2

We use a Server singleton class. Start the server either in mozmill.create or in cli (depending on context). We expose `baseurl` through 'extensions.mozmill.baseurl' which is exported globally in frame.js as `baseurl`.


marionette
===
Source: http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#661-673

There are a few distinct differences. Marionette has 1 marionette object which it can rely upon. Marionette save the state of the server on itself. Depending on the context in Mozmill we need to start the server in 2 places (Mozmill.create and cli). Because of this difference I've create a Server singleton class to be able to only start the server once and retrieve the status.

In Marionette the url is exposed as `marionette.baseurl` we expose this as `baseurl`.

baseurl
===
One other big difference is the docroot. Marionette has a `www` folder which it
exposes by default. We don't have this in mozmill.

The docroot can be set in the manifest files:
> [DEFAULT]
> baseurl = %path%

Issue here is that we get a list of _all_ manifest files. But we only want to instantiate Mozhttpd once, currently we use the first found manifest to set `baseurl`. This will not allow to customize it for deep tests.

This can also be superseded by the cli option --baseurl=%path%

mozmill-automation
===
We'll still need to patch mozmill-automation to read the baseurl from the manifest file. Tracked here: https://github.com/mozilla/mozmill-automation/issues/144

mutt
===
All mutt tests are passing

Performance: 8% faster
====
> mozmill -m firefox/tests/functional/manifest.ini -b /Applications/FirefoxNightly.app/
https.js: 414s
mozhttpd: 384s

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #7)
> http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/
> marionette/runner/base.py#661-673
> 
> There are a few distinct differences. Marionette has 1 marionette object
> which it can rely upon. Marionette save the state of the server on itself.
> Depending on the context in Mozmill we need to start the server in 2 places
> (Mozmill.create and cli). Because of this difference I've create a Server
> singleton class to be able to only start the server once and retrieve the
> status.

We can perfectly do the same in Mozmill, where we also have a single MozMill instance. It's __init__ method will always be called. So I don't see a difference. Also it uses --server-root as CLI option.

> baseurl
> ===
> One other big difference is the docroot. Marionette has a `www` folder which
> it
> exposes by default. We don't have this in mozmill.

Where is this 'www' folder located? Under each and every test directory?

> The docroot can be set in the manifest files:
> > [DEFAULT]
> > baseurl = %path%
> 
> Issue here is that we get a list of _all_ manifest files. But we only want
> to instantiate Mozhttpd once, currently we use the first found manifest to
> set `baseurl`. This will not allow to customize it for deep tests.

We do not have to instantiate mozhttpd for each test. What you want is to update the docroot variable on the current instance if necessary. In case of our tests it should always be the same. Something I would like to know is how to minimize the work for all the tests. Great would be to have a super manifest, and only that one holds the information, whereby lower-level manifests could overwrite this settig.

> mozmill-automation
> ===
> We'll still need to patch mozmill-automation to read the baseurl from the
> manifest file. Tracked here:
> https://github.com/mozilla/mozmill-automation/issues/144

This is not a requirement for the implementation of this bug. It's an additional enhancement, which can be done later and would allow us to push in an already existent URL.

> mutt
> ===
> All mutt tests are passing
> 
> Performance: 8% faster
> ====
> > mozmill -m firefox/tests/functional/manifest.ini -b /Applications/FirefoxNightly.app/
> https.js: 414s
> mozhttpd: 384s

Nice!

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8425388 [details] [diff] [review]
WIP 2

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

As it works we can see that all tests are passing. But the implementation needs to be refactored. Please see my inline comments.

::: mozmill/mozmill/__init__.py
@@ +36,5 @@
>  
>  # defaults
>  ADDONS = [extension_path, jsbridge.extension_path]
>  JSBRIDGE_TIMEOUT = 60.
> +MOZHTTPD_PORT = 43336

What happens if that port is already in use? E.g. that would block us in the future to run multiple tests in parallel on the same machine.

@@ +109,5 @@
>  
> +        # start mozhttp server
> +        server = Server()
> +        if not server.httpd:
> +            server.start_httpd(baseurl)

We don't want to create and start the server at this level. Especially not in two different places. Why don't you do this in the __init__ method of the MozMill class? Also we would loose any reference to the server instance.

@@ +125,5 @@
>          profile_args.setdefault('addons', []).extend(ADDONS)
>  
>          preferences = profile_args.setdefault('preferences', {})
>          if isinstance(preferences, dict):
> +            preferences['extensions.mozmill.baseurl'] = server.get_url()

The base url as set here is not interesting for the tests. What you want to have here is the actual URL as served by mozhttpd.

@@ +746,5 @@
>                           help="List test files that would be run, in order")
> +        group.add_option('--baseurl',
> +                         dest='baseurl',
> +                         default=None,
> +                         help="Base url for serving local content")

Why baseurl? As I have said we should be in sync with Marionette, which is using --server-root.

@@ +884,5 @@
> +
> +    def start_httpd(self, docroot=None):
> +        self.httpd = mozhttpd.MozHttpd(docroot=docroot, port=MOZHTTPD_PORT)
> +        self.httpd.start()
> +        return self.httpd.get_url()

We do not need this class when the HTTP server instance gets correctly created inside of the MozMill class.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +24,5 @@
>  Cu.import('resource://mozmill/stdlib/securable-module.js', securableModule);
>  
>  var uuidgen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>  
> +var baseurl = utils.getPreference('extensions.mozmill.baseurl', '');

We don't need this as a global variable of that file. It needs to be set as global on each imported module.

@@ +449,5 @@
>    this.testing = [];
>  }
>  
>  Collector.prototype.addHttpResource = function collector_addHttpResource(aDirectory, aPath) {
> +  return baseurl;

As mentioned in the other file already, this will not return the URL but the local path if specified.

Also when we keep this method for backward compat, please mark it as deprecated. We don't want to make use of it in the future.

::: mutt/mutt/mutt.py
@@ +266,5 @@
>      # Parse the manifest
>      mp = TestManifest(manifests=(options.manifest,), strict=False)
>  
> +    # Set up the baseurl
> +    options.baseurl = os.path.join(mp.rootdir,

Please not mangle with options.* manually. This object is created by the optparse module.

@@ +267,5 @@
>      mp = TestManifest(manifests=(options.manifest,), strict=False)
>  
> +    # Set up the baseurl
> +    options.baseurl = os.path.join(mp.rootdir,
> +                                   mp.get('baseurl')[0])

Shouldn't this be hard-coded given that it will not change?

Comment 10

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> > baseurl
> > ===
> > One other big difference is the docroot. Marionette has a `www` folder which
> > it
> > exposes by default. We don't have this in mozmill.
> 
> Where is this 'www' folder located? Under each and every test directory?

No, it lives in marionette proper:
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/www

We do have the mutt data/ folder, but since mutt is only dependency for mozmill I wouldn't go so far as to include a "default" webserver folder based on that.

> > The docroot can be set in the manifest files:
> > > [DEFAULT]
> > > baseurl = %path%
> > 
> > Issue here is that we get a list of _all_ manifest files. But we only want
> > to instantiate Mozhttpd once, currently we use the first found manifest to
> > set `baseurl`. This will not allow to customize it for deep tests.
> 
> We do not have to instantiate mozhttpd for each test. What you want is to
> update the docroot variable on the current instance if necessary. In case of
> our tests it should always be the same. Something I would like to know is
> how to minimize the work for all the tests. Great would be to have a super
> manifest, and only that one holds the information, whereby lower-level
> manifests could overwrite this settig.

That sounds good in theory, the problem lies when wanting to only run a deep test.
I'll check to see if we can safely override the docroot (my initial research revealed that we will need to restart mozhttpd if we want to change the docroot). I'll check if there's a way.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #10)
> > Where is this 'www' folder located? Under each and every test directory?
> 
> No, it lives in marionette proper:
> http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/
> marionette/www

This is for unit tests of Marionette, right? But my question was how tests for specific Firefox features are written, and where the test data is stored.

> We do have the mutt data/ folder, but since mutt is only dependency for
> mozmill I wouldn't go so far as to include a "default" webserver folder
> based on that.

Mutt is not a dependency for Mozmill, but Mutt depends on Mozmill. So it's the other way around. By running Mutt tests it should be clear that we want to use the local test data files. Nothing else should be used here. So it can be hard-coded.

> I'll check to see if we can safely override the docroot (my initial research
> revealed that we will need to restart mozhttpd if we want to change the
> docroot). I'll check if there's a way.

You know that all the tools are managed by ourselves, and enhancements are welcome if helpful and wanted. We should not start doing workarounds if something else might be better.

Comment 12

[Andrei Eftimie]

Attachment: WIP 3

Another update.

1. We now start the server directly on Mozmill
I initially tried this, but because we wanted to use a pref to export the server url and the prefs were set on mozrunner (before we initialized mozmill) this was not working. I now changed this and we set the pref always before starting the runner (not at init time).

2. cli option is now --server-root
3. added support for multiple ports (if the default one is in use)
4. hardcoded mutt server-root
5. marked addHttpdResource as deprecated (for this moved the logDeprecated methods in utils)
6. addHttpdResource also returns the path if specified

Still todo:
a. more reliable way of reading the root from manifest files
b. and check if we can somehow change the docroot without restarting the server
(I have a strong feeling that this will not be possible _without_ a restart of the webserver)
c. add some more tests, probably a python test to test cli and manifest server-root paths

Comment 13

[Andrei Eftimie]

Just a small update:
we can dynamically update the docroot by updating mozhttpd.handler_class.docroot

Now to figure out how to properly architect setting up the server-root in the manifest files so that we can override them.

Comment 14

[Andrei Eftimie]

Attachment: 0004-Bug-754847-Implement-mozhttpd.-r-hskupin.patch

Not sure if this is completely ready, but it is in a much better state now.
I'd like another set of eyes on this since it is changing some core functionality and its highly likely I might have missed some cases.

All mutt tests are passing, I have 5 failures in a functional testrun (with mozmill and the --server-root cli option). I am investigating to see if these are directly related, or have another cause.

We now dynamically change the docroot for each test that has server-root set in its manifest file. As well as various small fixes and improvements.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Today we had the topic about mozhttpd in #ateam. Just by accident. So I got the hint to have a look at https://github.com/w3c/wptserve, which is a way better web server with lots of more capabilities compared to mozhttpd. The latter is even not bundled to Marionette, which I thought it was. So we might be free using any other kind of tool here.

Comment 16

[Andrei Eftimie]

Oh, at a quick glance wptserve does indeed look nice.
Might be a good fit for our needs. I'll check it out more in depth.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Yeah, and if it fits, replacing the mozhttpd code with wptserve should be easily done.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8430033 [details] [diff] [review]
0004-Bug-754847-Implement-mozhttpd.-r-hskupin.patch

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

::: mozmill/mozmill/__init__.py
@@ +36,5 @@
>  
>  # defaults
>  ADDONS = [extension_path, jsbridge.extension_path]
>  JSBRIDGE_TIMEOUT = 60.
> +MOZHTTPD_DEFAULT_PORT = 43336

Maybe we should drop that and let the server automatically determine a free port. It would fail if this port is in use. This might be the problem we currently have in Mozmill and httpd.js, if an instance of Firefox stays open. In such a case it might be that the port is in use, but httpd.js cannot respond. So we are timing out in the page load.

@@ +154,5 @@
>  
>          Keyword arguments:
>          jsbridge_timeout -- How long to wait without a jsbridge communication
>          handlers -- pluggable event handlers
>          screenshots_path -- Path where screenshots will be saved

You missed to add the new argument.

@@ +167,5 @@
> +        while self.httpd.get_url() is None:
> +            try:
> +                self.httpd.start()
> +            except:
> +                self.httpd.port += 1

That while we could also get rid of by letting mozhttpd chose a free port.

@@ +336,4 @@
>              self.runner.start(debug_args=self.debugger,
>                                interactive=self.interactive)
> +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> +                                                 self.httpd.get_url()})

Shouldn't this be set before calling `runner.start()`? It smells like a race condition to me.

@@ +381,5 @@
>          try:
> +            # set docroot
> +            if 'server-root' in test:
> +                self.httpd.handler_class.docroot = os.path.join(test['here'],
> +                                                                test['server-root'])

Lets make sure this is allowed. Especially when you have a look at wptserve.

@@ +655,5 @@
>                                       strict=False)
>  
> +        # get server_root from cli argument or manifest
> +        self.server_root = None
> +        if self.options.server_root is not None:

`if self.options.server_root` is enough here.

@@ +746,5 @@
>                           dest='list_tests',
>                           action='store_true',
>                           default=False,
>                           help="List test files that would be run, in order")
> +        group.add_option('--server-root',

Please keep the alphabetical order as usual.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +449,5 @@
>  }
>  
>  Collector.prototype.addHttpResource = function collector_addHttpResource(aDirectory, aPath) {
> +  utils.logDeprecated('Collector.addHttpResource', 'Define the root in the manifest file');
> +  let baseurl = utils.getPreference('extensions.mozmill.baseurl', '');

Make this pref a constant at the top of the file. Also what about storing this setting as class member? That way it can be re-used below, and no need to retrieve it again from the preference.

@@ +450,5 @@
>  
>  Collector.prototype.addHttpResource = function collector_addHttpResource(aDirectory, aPath) {
> +  utils.logDeprecated('Collector.addHttpResource', 'Define the root in the manifest file');
> +  let baseurl = utils.getPreference('extensions.mozmill.baseurl', '');
> +  return (aPath) ? baseurl + aPath : baseurl;

nit: no need for the brackets around aPath, but nice for the concatenation.

::: mutt/mutt/mutt.py
@@ +47,5 @@
>            action="store_true",
>            help="Isolation mode (restart between each Mozmill test)")),
> +    (("-s", "--server-root"),
> +     dict(dest="server_root",
> +          default=os.path.join(os.path.dirname(os.path.realpath(__file__)),

Does abspath() not work here? We use it in mozbase on various places.

@@ +49,5 @@
> +    (("-s", "--server-root"),
> +     dict(dest="server_root",
> +          default=os.path.join(os.path.dirname(os.path.realpath(__file__)),
> +                                               'tests','data'),
> +          help="Document root used by Mozhttpd")),

I would not call out the type of HTTP server, just say 'HTTP server'.

Comment 19

[Andrei Eftimie]

Attachment: 0005-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Comment on attachment 8430033 [details] [diff] [review]
> 0004-Bug-754847-Implement-mozhttpd.-r-hskupin.patch
> 
> Review of attachment 8430033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozmill/mozmill/__init__.py
> @@ +36,5 @@
> >  
> >  # defaults
> >  ADDONS = [extension_path, jsbridge.extension_path]
> >  JSBRIDGE_TIMEOUT = 60.
> > +MOZHTTPD_DEFAULT_PORT = 43336
> 
> Maybe we should drop that and let the server automatically determine a free
> port. It would fail if this port is in use. This might be the problem we
> currently have in Mozmill and httpd.js, if an instance of Firefox stays
> open. In such a case it might be that the port is in use, but httpd.js
> cannot respond. So we are timing out in the page load.
I don't think this is the current problem since we have the same logic
implemented in httpd.js at the moment. See https://github.com/mozilla/mozmill/blob/hotfix-2.0/mozmill/mozmill/extension/resource/modules/frame.js#L610-L628

If the port we specify is in use, we fail, so we try another one. I've
implemented the same logic as to increment the port by 1. I've tested this and
we can start 2 mozmill instances (with 2 servers) simultaneusly. I wonder if we 
can test this. I think it should be possible with a python test...

> @@ +154,5 @@
> >  
> >          Keyword arguments:
> >          jsbridge_timeout -- How long to wait without a jsbridge communication
> >          handlers -- pluggable event handlers
> >          screenshots_path -- Path where screenshots will be saved
> 
> You missed to add the new argument.

Done.

> @@ +167,5 @@
> > +        while self.httpd.get_url() is None:
> > +            try:
> > +                self.httpd.start()
> > +            except:
> > +                self.httpd.port += 1
> 
> That while we could also get rid of by letting mozhttpd chose a free port.
Neither mozhttpd nor wptserve don't have this functionality built in, so we have
to handle it.

> @@ +336,4 @@
> >              self.runner.start(debug_args=self.debugger,
> >                                interactive=self.interactive)
> > +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> > +                                                 self.httpd.get_url()})
> 
> Shouldn't this be set before calling `runner.start()`? It smells like a race
> condition to me.
Good call, implemented it and it works fine.

> @@ +381,5 @@
> >          try:
> > +            # set docroot
> > +            if 'server-root' in test:
> > +                self.httpd.handler_class.docroot = os.path.join(test['here'],
> > +                                                                test['server-root'])
> 
> Lets make sure this is allowed. Especially when you have a look at wptserve.

I haven't found any information stating that this is _allowed_ or not. It does 
work. I've spoken to jgraham about this on IRC, and this "feature" is not
officially supported. It _might_ break in future versions. And that we should
not depend on it if we can avoid it.

I'm leaving this in at the moment. We will be notified if we'll try to update
wptserve and this doesn't work anymore as we have a mutt test.


> @@ +655,5 @@
> >                                       strict=False)
> >  
> > +        # get server_root from cli argument or manifest
> > +        self.server_root = None
> > +        if self.options.server_root is not None:
> 
> `if self.options.server_root` is enough here.
Done

> @@ +746,5 @@
> >                           dest='list_tests',
> >                           action='store_true',
> >                           default=False,
> >                           help="List test files that would be run, in order")
> > +        group.add_option('--server-root',
> 
> Please keep the alphabetical order as usual.
Done

> ::: mozmill/mozmill/extension/resource/modules/frame.js
> @@ +449,5 @@
> >  }
> >  
> >  Collector.prototype.addHttpResource = function collector_addHttpResource(aDirectory, aPath) {
> > +  utils.logDeprecated('Collector.addHttpResource', 'Define the root in the manifest file');
> > +  let baseurl = utils.getPreference('extensions.mozmill.baseurl', '');
> 
> Make this pref a constant at the top of the file. Also what about storing
> this setting as class member? That way it can be re-used below, and no need
> to retrieve it again from the preference.
Done

> @@ +450,5 @@
> >  
> >  Collector.prototype.addHttpResource = function collector_addHttpResource(aDirectory, aPath) {
> > +  utils.logDeprecated('Collector.addHttpResource', 'Define the root in the manifest file');
> > +  let baseurl = utils.getPreference('extensions.mozmill.baseurl', '');
> > +  return (aPath) ? baseurl + aPath : baseurl;
> 
> nit: no need for the brackets around aPath, but nice for the concatenation.
Done

> ::: mutt/mutt/mutt.py
> @@ +47,5 @@
> >            action="store_true",
> >            help="Isolation mode (restart between each Mozmill test)")),
> > +    (("-s", "--server-root"),
> > +     dict(dest="server_root",
> > +          default=os.path.join(os.path.dirname(os.path.realpath(__file__)),
> 
> Does abspath() not work here? We use it in mozbase on various places.
It works. Changed.

> @@ +49,5 @@
> > +    (("-s", "--server-root"),
> > +     dict(dest="server_root",
> > +          default=os.path.join(os.path.dirname(os.path.realpath(__file__)),
> > +                                               'tests','data'),
> > +          help="Document root used by Mozhttpd")),
> 
> I would not call out the type of HTTP server, just say 'HTTP server'.
Done. Replaced all instances of "mozhttpd" with "web server" or "httpd"

======

We have 1 failure in js/testWebServer/testMultipleLoads/test2.js 
> "Page has been loaded from the expected source. - 'http-on-examine-response' should equal 'http-on-examine-cached-response'"
It seems we always get 200 as a response and we expect a 304 here.
(We would get 304 for a cached page from httpd.js and from mozhttpd).
I'll have to dig a bit to see exactly why (might be that this is expected) as we 
can manually set the response type via pipes...

I'm not asking for a review yet, as I want to update my patch for manifestparser from bug 1023790 and do some testing with both first.

Comment 20

[Andrei Eftimie]

In a future version bug 1025897 might be nice to use to get a free port when it gets implemented.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Depends on its implementation. It could still raise a race condition.

Comment 22

[Andrei Eftimie]

Attachment: 0006-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

Updated patch.

Some issues:

1) The cli option --server-root=%path% should have priority over any option set in the manifest files. This invalidates my mutt tests in /mutt/mutt/tests/js/testWebServer/testChangeServerRoot/ as they want to change their option. Not sure how to overcome this. WIll probably need to change this to a python test.

2) Had to revert to specifying a port manually, I wasn't able to run mutt python tests otherwise (wptserve fails if I try to create a new instance on the same port). 

3) Changed the way we compute the server-root based on the the full vs relative path. Since the server-root option cascades through all tests files, yet they all have a different absolute path, we would fail to have the correct path... because of this I would also like to drop the hard blocker on bug 1023913 and mark that as a nice-to-have enhancement. Because of this limitation bug 1023913 isn't helpful right now.

The one downside is that while this all works for full testruns, for manually running a subset of tests we would need to pass in the cli option for the server root via --server-root=%path%

Comment 23

[Andrei Eftimie]

We will have to wait for a new release of wptserve (specifically we _want_ xpi content type support by default).

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #22)
> 1) The cli option --server-root=%path% should have priority over any option
> set in the manifest files. This invalidates my mutt tests in
> /mutt/mutt/tests/js/testWebServer/testChangeServerRoot/ as they want to
> change their option. Not sure how to overcome this. WIll probably need to
> change this to a python test.

Yes, that's the only option you have here. Btw, I would implement all the tests for this bug as Python tests, whereby the above one should be a CLI test.

> 2) Had to revert to specifying a port manually, I wasn't able to run mutt
> python tests otherwise (wptserve fails if I try to create a new instance on
> the same port). 

Have you closed the formerly opened socket used to check if the port is free? If not, you have to do that.

> 3) Changed the way we compute the server-root based on the the full vs
> relative path. Since the server-root option cascades through all tests
> files, yet they all have a different absolute path, we would fail to have
> the correct path... because of this I would also like to drop the hard
> blocker on bug 1023913 and mark that as a nice-to-have enhancement. Because
> of this limitation bug 1023913 isn't helpful right now.

We might wanna do that. So getting this patch landed on github, and waiting for bug 1023913 landed, before updating any of our mozmill tests.

> The one downside is that while this all works for full testruns, for
> manually running a subset of tests we would need to pass in the cli option
> for the server root via --server-root=%path%

Well, we would like to go away with -t, so having it in the manifest, even in everyone for now, would fix it. Right?

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8445166 [details] [diff] [review]
0006-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

::: mozmill/mozmill/__init__.py
@@ +21,5 @@
>  
>  import jsbridge
>  from jsbridge.network import JSBridgeDisconnectError
>  
> +from wptserve import server

Why are you creating a separate section for this import? It should be part of the global imports.

@@ +36,5 @@
>  
>  # defaults
>  ADDONS = [extension_path, jsbridge.extension_path]
>  JSBRIDGE_TIMEOUT = 60.
> +WEB_SERVER_PORT = 43336

Lets file a follow-up bug to get it auto-detect a free port and make it dependent on this bug.

@@ +164,5 @@
>  
> +        # start mozhttp server
> +        self.server_root = server_root
> +        url = None
> +        port = WEB_SERVER_PORT

url is only used inside the loop. No need to declare it. You can break the loop with 'break' if it was successful.

@@ +339,5 @@
>              if self.shutdownMode.get('resetProfile'):
>                  # reset the profile
>                  self.runner.reset()
> +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> +                                                 self.httpd.get_url()})

I'm not happy with that line. Why it has to be done here? Why can't it be added to the same places as we have for the e.g. JS bridge port?

@@ +386,5 @@
>          """
>          try:
> +            # set docroot
> +            if not self.server_root and 'server-root' in test:
> +                self.httpd.router.doc_root = os.path.abspath(

I don't see that this is a public API. So we cannot use that:
http://wptserve.readthedocs.org/en/latest/router.html#module-wptserve.router

@@ +662,5 @@
>  
> +        # get server_root from cli
> +        self.server_root = None
> +        if self.options.server_root:
> +            self.server_root = self.options.server_root

Why not: 'self.server_root = self.options.server_root or None'?

::: mutt/mutt/mutt.py
@@ +49,5 @@
> +    (("-s", "--server-root"),
> +     dict(dest="server_root",
> +          default=os.path.join(os.path.dirname(os.path.abspath(__file__)),
> +                                               'tests','data'),
> +          help="Document root used by Mozhttpd")),

Why do we need this as a CLI option? It's fixed and will never change.

::: mutt/mutt/tests/js/metro/testTouchEvents/testDoubleTap.js
@@ +3,5 @@
>   * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const TEST_DATA = baseurl + "complex.html";

Can you please create one test, which checks the backward capability for addHttpResource?

::: mutt/mutt/tests/python/test_slow_pageload_on_startup.py
@@ +15,5 @@
>  
>      def do_test(self, relative_test_path, passes=0, fails=0, skips=0):
>          testpath = os.path.join(os.path.dirname(os.path.abspath(__file__)),
>                                  relative_test_path)
> +        server_root = os.path.join(os.path.dirname(os.path.abspath(__file__)),

I would suggest to create 'here' at the top of the file.

Comment 26

[Andrei Eftimie]

Attachment: 0007-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

(In reply to Henrik Skupin (:whimboo) from comment #24)
> (In reply to Andrei Eftimie from comment #22)
> > 1) The cli option --server-root=%path% should have priority over any option
> > set in the manifest files. This invalidates my mutt tests in
> > /mutt/mutt/tests/js/testWebServer/testChangeServerRoot/ as they want to
> > change their option. Not sure how to overcome this. WIll probably need to
> > change this to a python test.
> 
> Yes, that's the only option you have here. Btw, I would implement all the
> tests for this bug as Python tests, whereby the above one should be a CLI
> test.

Actually I fixed this by mutt not explicitly creating a Mozmill instance with
the server-root option. We have this set up in the root manifest file, we use
that and it works great.

> > 2) Had to revert to specifying a port manually, I wasn't able to run mutt
> > python tests otherwise (wptserve fails if I try to create a new instance on
> > the same port). 
> 
> Have you closed the formerly opened socket used to check if the port is
> free? If not, you have to do that.

Good point. I now stop the server when we shut down the harness. Tested and that
fixes the issue.

But I've left this code so that we can run multiple mozmill instances
simultaneuosly.

> > 3) Changed the way we compute the server-root based on the the full vs
> > relative path. Since the server-root option cascades through all tests
> > files, yet they all have a different absolute path, we would fail to have
> > the correct path... because of this I would also like to drop the hard
> > blocker on bug 1023913 and mark that as a nice-to-have enhancement. Because
> > of this limitation bug 1023913 isn't helpful right now.
> 
> We might wanna do that. So getting this patch landed on github, and waiting
> for bug 1023913 landed, before updating any of our mozmill tests.

Sounds good to me. We can still update mozmill tests to have the server-root
set up in the base manifest file. That is backwards compatible.

Adding [parent:] includes can be done at a later date.

> > The one downside is that while this all works for full testruns, for
> > manually running a subset of tests we would need to pass in the cli option
> > for the server root via --server-root=%path%
> 
> Well, we would like to go away with -t, so having it in the manifest, even
> in everyone for now, would fix it. Right?

Right now we will need it for both -t and -m.
After bug 1023913 lands (and if we find a good way to have correct paths), -m
should work everywhere.


===

There still 1 failure in mutt/mutt/tests/js/testWebServer/testMultipleLoads/
After we load a page once we expect the server to issue us a 304 and Firefox to load the page from cache. It seems wptserve doesn't do that. I'll dig around it more, and will probably file an issue to see if it will get support for cached content.

Comment 27

[Andrei Eftimie]

Comment on attachment 8446424 [details] [diff] [review]
0007-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

Seems I just missed a more deep review. Removing the flash until I update.

Comment 28

[Andrei Eftimie]

Attachment: 0008-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 8445166 [details] [diff] [review]
> 0006-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch
>
> Review of attachment 8445166 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +339,5 @@
> >              if self.shutdownMode.get('resetProfile'):
> >                  # reset the profile
> >                  self.runner.reset()
> > +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> > +                                                 self.httpd.get_url()})
>
> I'm not happy with that line. Why it has to be done here? Why can't it be
> added to the same places as we have for the e.g. JS bridge port?

These are set in 2 places. Once in Mozmill.create() and once in
CLI.profile_args(). Both these are executed _before_ we start the server. Since
the server reference is saved on the Mozmill object I haven't found a way to
start the server _before_ we do any of these.

I'm open to any suggestion here as this is a chicken & egg problem.

> @@ +386,5 @@
> >          """
> >          try:
> > +            # set docroot
> > +            if not self.server_root and 'server-root' in test:
> > +                self.httpd.router.doc_root = os.path.abspath(
>
> I don't see that this is a public API. So we cannot use that:
> http://wptserve.readthedocs.org/en/latest/router.html#module-wptserve.router

As discussed I've filed an issue against wptserve to see if this can become a
supported feature. We do not actually _need_ this right now. We do serve all
files from the same place.

> @@ +662,5 @@
> >
> > +        # get server_root from cli
> > +        self.server_root = None
> > +        if self.options.server_root:
> > +            self.server_root = self.options.server_root
>
> Why not: 'self.server_root = self.options.server_root or None'?

I knew I was missing a pythonic way for this. Thanks!

> ::: mutt/mutt/mutt.py
> @@ +49,5 @@
> > +    (("-s", "--server-root"),
> > +     dict(dest="server_root",
> > +          default=os.path.join(os.path.dirname(os.path.abspath(__file__)),
> > +                                               'tests','data'),
> > +          help="Document root used by Mozhttpd")),
>
> Why do we need this as a CLI option? It's fixed and will never change.

Right. I took this out.
The only change in mutt now is in the manifest.

> ::: mutt/mutt/tests/js/metro/testTouchEvents/testDoubleTap.js
> @@ +3,5 @@
> >   * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> >
> >  "use strict";
> >
> > +const TEST_DATA = baseurl + "complex.html";
>
> Can you please create one test, which checks the backward capability for
> addHttpResource?

Done

> ::: mutt/mutt/tests/python/test_slow_pageload_on_startup.py
> @@ +15,5 @@
> >
> >      def do_test(self, relative_test_path, passes=0, fails=0, skips=0):
> >          testpath = os.path.join(os.path.dirname(os.path.abspath(__file__)),
> >                                  relative_test_path)
> > +        server_root = os.path.join(os.path.dirname(os.path.abspath(__file__)),
>
> I would suggest to create 'here' at the top of the file.

Done

====

The new geolocation test fails, seems we also miss the correct MIME type for
JSON files. I'm in the process of updating https://github.com/w3c/wptserve/pull/31 to get this as well
(haven't yet gotten any response for any PR's).

Comment 29

[Andrei Eftimie]

(In reply to Andrei Eftimie from comment #28)
> The new geolocation test fails, seems we also miss the correct MIME type for
> JSON files. I'm in the process of updating
> https://github.com/w3c/wptserve/pull/31 to get this as well
> (haven't yet gotten any response for any PR's).

Ugh, it actually appears that "gls" doesn't correctly talk to wptserve. The JSON resource is available at the requested address (even with the correct mime-type). Yet we still get 404.
> *** WIFI GEO: Sending request: http://localhost:43336/geolocation/locations/mozilla_san_francisco.json
> *** WIFI GEO: sending {}
> *** WIFI GEO: gls returned status: 404 --> {"error":{"message":"","code":404}}
Not sure _where_ this communication is broken.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #28)
> > >              if self.shutdownMode.get('resetProfile'):
> > >                  # reset the profile
> > >                  self.runner.reset()
> > > +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> > > +                                                 self.httpd.get_url()})
> >
> > I'm not happy with that line. Why it has to be done here? Why can't it be
> > added to the same places as we have for the e.g. JS bridge port?
> 
> These are set in 2 places. Once in Mozmill.create() and once in
> CLI.profile_args(). Both these are executed _before_ we start the server.
> Since
> the server reference is saved on the Mozmill object I haven't found a way to
> start the server _before_ we do any of these.

So will this URL ever change? I mean, shouldn't it be always the same even we change the docroot? The only thing which can change here is the port, right?

> > I don't see that this is a public API. So we cannot use that:
> > http://wptserve.readthedocs.org/en/latest/router.html#module-wptserve.router
> 
> As discussed I've filed an issue against wptserve to see if this can become a
> supported feature. We do not actually _need_ this right now. We do serve all
> files from the same place.

This lacks a link to the issue you filed.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #29)
> > *** WIFI GEO: gls returned status: 404 --> {"error":{"message":"","code":404}}
> Not sure _where_ this communication is broken.

Very bad output here, with even not having an error message. :/ Try to create a NSPR HTTP log, which might help here.

Comment 32

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #30)
> (In reply to Andrei Eftimie from comment #28)
> > > >              if self.shutdownMode.get('resetProfile'):
> > > >                  # reset the profile
> > > >                  self.runner.reset()
> > > > +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> > > > +                                                 self.httpd.get_url()})
> > >
> > > I'm not happy with that line. Why it has to be done here? Why can't it be
> > > added to the same places as we have for the e.g. JS bridge port?
> > 
> > These are set in 2 places. Once in Mozmill.create() and once in
> > CLI.profile_args(). Both these are executed _before_ we start the server.
> > Since
> > the server reference is saved on the Mozmill object I haven't found a way to
> > start the server _before_ we do any of these.
> 
> So will this URL ever change? I mean, shouldn't it be always the same even
> we change the docroot? The only thing which can change here is the port,
> right?

Uhg, I replied earlier, seems my message didn't wound up on bugzilla :(.

It shouldn't change once we have the server started. Yes, changing the docroot doesn't have any effect on the address. We pass in the hostname as `localhost` (to preserve even more backwards compatibility). In theory we _could_ manually craft the URL. But I wouldn't give up on the canonical get_url() method. And we can only invoke this once the webserver is running.

And we want the webserver object to be bound to the Mozmill instance. I guess we might also be able to start the webserver prior to instantiating mozmill, and get the reference to the webserver in mozmill. This way we can get the url before we need to start the runner... Not sure if this is for the better.

> > > I don't see that this is a public API. So we cannot use that:
> > > http://wptserve.readthedocs.org/en/latest/router.html#module-wptserve.router
> > 
> > As discussed I've filed an issue against wptserve to see if this can become a
> > supported feature. We do not actually _need_ this right now. We do serve all
> > files from the same place.
> 
> This lacks a link to the issue you filed.
Right, this is the issue: https://github.com/w3c/wptserve/issues/32

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #32)
> It shouldn't change once we have the server started. Yes, changing the
> docroot doesn't have any effect on the address. We pass in the hostname as
> `localhost` (to preserve even more backwards compatibility). In theory we
> _could_ manually craft the URL. But I wouldn't give up on the canonical
> get_url() method. And we can only invoke this once the webserver is running.

Sure, but this location seems to be inappropriate for placing this call to get_url(). We don't want to execute it each time the application gets restarted. Do it at the very beginning, when instantiating the web server. Add the pref to the profile instance, and this class should take care of setting it again after resetting the profile.

Comment 34

[Andrei Eftimie]

Attachment: 0009-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

This is a good update.

1) We remove the dependency on a new version of wptserve by monkey patching wptserve's content_types array with our own needed values.

2) I've created 2 methods on Mozmill to encapsulate starting and stopping the webserver.

3) I've set the webserver URL preference right after starting the webserver and saved its value for when we need it after a profile reset. 

====

Issues we still have:
a) geolocation JSON coordinates fail // I'll do some logging as suggested
The new geolocation tests are affected

b) Cached response mutt test fails: js/testWebServer/testMultipleLoads/test2.js
we still get the page with a 200 response instead of an expected 304

c) (minor) whenever we attempt to start the server we get some output in the console:
> ^/(.*)$
> ^/(.*)\.asis$
> ^/(.*)\.py$\
> No handlers could be found for logger "wptserve"
I have previously filed: https://github.com/w3c/wptserve/issues/29
I don't see an easy way of disabling these at the moment.
Not sure yet about the "logger" handler.

--

I also have a few mutt python tests failing. But I have failures even without this patch. WIll check to make sure none are related to this and file appropriate bugs.

I haven't tested latest changes cross-platform (only OSX atm).

Comment 35

[Andrei Eftimie]

The problem with retrieving the JSON geolocation is that the request is xhr via POST. Swapped it  to GET and the tests are passing. I'll look into why wptserve fails to deliver this when it's requested via POST.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8448025 [details] [diff] [review]
0009-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

::: mozmill/mozmill/__init__.py
@@ +155,5 @@
>          Keyword arguments:
>          jsbridge_timeout -- How long to wait without a jsbridge communication
>          handlers -- pluggable event handlers
>          screenshots_path -- Path where screenshots will be saved
> +        server_root -- Path where to serve files from

it would add 'testcase files'

@@ +162,5 @@
>          # the MozRunner
>          self.runner = runner
>  
> +        # start the webserver
> +        self.webserver_start(server_root)

I would call it http_server given that we are in a code module and not writing documentation. Technical descriptions should be preferred then. I don't think the line then needs a comment. Also shouldn't we store server_root?

@@ +328,5 @@
>              if self.shutdownMode.get('resetProfile'):
>                  # reset the profile
>                  self.runner.reset()
> +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> +                                                 self.web_server_url})

Again, why does mozprofile not set this pref automatically when the profile gets reset? It should usually restore all the pref it knows about.

@@ +377,5 @@
> +            # set docroot
> +            if not self.server_root and 'server-root' in test:
> +                self.httpd.router.doc_root = os.path.abspath(
> +                    os.path.join(test['path'].split(test['relpath'])[0],
> +                                 test['server-root']))

I would construct the path in its own line to improve readability here.

@@ +604,5 @@
> +        # https://github.com/w3c/wptserve/pull/31
> +        content_types = wptserve.utils.invert_dict({
> +            'application/json': ['json'],
> +            'application/x-xpinstall': ['xpi'],})
> +        wptserve.constants.content_types.update(content_types)

You know that I'm not a fan of munging with private/protected class members. This is one more instance of it, which I don't see as solved correctly.

Why can't we use a __dir__.headers file for folders with files of to be added content types, as described at the following page. It would be similar to a .htaccess file for Apache.

http://wptserve.readthedocs.org/en/latest/handlers.html#file-handlers

@@ +607,5 @@
> +            'application/x-xpinstall': ['xpi'],})
> +        wptserve.constants.content_types.update(content_types)
> +
> +        # start the server
> +        self.server_root = server_root

This assignment has to take place in the constructor, and should not be delegated to a class method.

@@ +611,5 @@
> +        self.server_root = server_root
> +        port = WEB_SERVER_PORT
> +        while True:
> +            try:
> +                self.httpd = wptserve.server.WebTestHttpd(

lets call it self.http_server. The 'd' suffix would mean it is a daemon, which indeed it is not.

@@ +620,5 @@
> +                if self.httpd.get_url():
> +                    break
> +            except:
> +                port += 1
> +        self.web_server_url = self.httpd.get_url()

Is there a reason we have to store it? We could simply call get_url() when it is necessary.

@@ +622,5 @@
> +            except:
> +                port += 1
> +        self.web_server_url = self.httpd.get_url()
> +
> +        # expose the url as a pref

nit: URL

@@ +624,5 @@
> +        self.web_server_url = self.httpd.get_url()
> +
> +        # expose the url as a pref
> +        self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> +                                             self.web_server_url})

Still not sure if this is the right way to set default preferences. As you have seen it get lost after a reset. So please check if we can set it so it sticks.

@@ +687,5 @@
>          self.manifest = TestManifest(manifests=self.options.manifests,
>                                       strict=False)
>  
> +        # get server_root from cli
> +        self.server_root = self.options.server_root or None

This line can be removed. It's not necessary. You already specify None as default in optparse.

@@ +788,5 @@
>                           help='Path of directory to use for screenshots')
> +        group.add_option('--server-root',
> +                         dest='server_root',
> +                         default=None,
> +                         help="Document root for serving local content")

nit: I would say 'serving local testcases'. Also keep to single quotes.

@@ +859,5 @@
>          mozmill = MozMill(runner, self.jsbridge_port,
>                            jsbridge_timeout=self.options.timeout,
>                            handlers=self.event_handlers,
> +                          screenshots_path=self.options.screenshots_path,
> +                          server_root=self.server_root)

`=self.options.server_root`

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +502,5 @@
>    return true;
>  }
>  
> +let logDeprecated = utils.logDeprecated;
> +let logDeprecatedAssert = utils.logDeprecatedAssert;

Instead of defining it could you use utils.logDeprecated directly for any instance of that file? Same for the assert.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +33,5 @@
>  var mozmill = undefined;
>  var mozelement = undefined;
>  var modules = undefined;
>  
> +var baseurl = utils.getPreference('extensions.mozmill.baseurl', '');

I don't see why this needs to be global. Make it a property of Collector, or retrieve the value in its constructor.

@@ +450,5 @@
>    this.testing = [];
>  }
>  
>  Collector.prototype.addHttpResource = function collector_addHttpResource(aDirectory, aPath) {
> +  utils.logDeprecated('Collector.addHttpResource', 'Define the root in the manifest file');

nit: 'Define the base_url as server_root...'

::: mutt/mutt/tests/js/metro/testTouchEvents/testDoubleTap.js
@@ +3,5 @@
>   * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const TEST_DATA = baseurl + "complex.html";

Lovely change! What an improvement!

::: mutt/mutt/tests/js/testController/manifest.ini
@@ -1,2 @@
> -[DEFAULT]
> -type = javascript

Why do you remove that? It's necessary for Mutt to determine the type of test.

::: mutt/mutt/tests/js/testFrame/testHttpd/manifest.ini
@@ -1,5 @@
> -[DEFAULT]
> -type = javascript
> -
> -[testMultipleLoads.js]
> -[testRestartAvailability.js]

The functionality of the http server is provided via frames.js, so please keep the tests in this folder.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Henrik Skupin (:whimboo) from comment #36)
> > +        # https://github.com/w3c/wptserve/pull/31
> > +        content_types = wptserve.utils.invert_dict({
> > +            'application/json': ['json'],
> > +            'application/x-xpinstall': ['xpi'],})
> > +        wptserve.constants.content_types.update(content_types)
> 
> You know that I'm not a fan of munging with private/protected class members.
> This is one more instance of it, which I don't see as solved correctly.
> 
> Why can't we use a __dir__.headers file for folders with files of to be
> added content types, as described at the following page. It would be similar
> to a .htaccess file for Apache.
> 
> http://wptserve.readthedocs.org/en/latest/handlers.html#file-handlers

Instead of spending hours in figuring out how to do that, I simply talked to jgraham on IRC. Within 3 minutes he landed your PR. We can have a new release of wptserve whenever we want to, so please speak up on IRC.

Please keep that in mind for the future. Only with communication we can ensure to get all of our tasks done, and don't loose time in implementing workarounds.

Comment 38

[Andrei Eftimie]

Attachment: 0010-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

This is not a final patch. I still have a couple of items on my todo list:
1) ensure all is working fine across platforms (I haven't yet tested windows, I don't forsee any problems, but anything is possible)
2) there's still an item from last review that I should _try_ finding a solution to: to set the `baseurl` pref together with the rest of the prefs set on the runner (not sure if feasible since we start the server _after_ those have been set)
3) mutt test testFrame/testHttpd/testMultipleLoads/test2.js is failing because the page doesn't get delivered from cache (we get 200 response when we would expect a 304)

Below are the review issues addressed:
(In reply to Henrik Skupin (:whimboo) from comment #36)
> Comment on attachment 8448025 [details] [diff] [review]
> 0009-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch
>
> Review of attachment 8448025 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mozmill/mozmill/__init__.py
> @@ +155,5 @@
> >          Keyword arguments:
> >          jsbridge_timeout -- How long to wait without a jsbridge communication
> >          handlers -- pluggable event handlers
> >          screenshots_path -- Path where screenshots will be saved
> > +        server_root -- Path where to serve files from
>
> it would add 'testcase files'
Done

> @@ +162,5 @@
> >          # the MozRunner
> >          self.runner = runner
> >
> > +        # start the webserver
> > +        self.webserver_start(server_root)
>
> I would call it http_server given that we are in a code module and not
> writing documentation. Technical descriptions should be preferred then. I
> don't think the line then needs a comment. Also shouldn't we store
> server_root?
Sure. Done.

> @@ +328,5 @@
> >              if self.shutdownMode.get('resetProfile'):
> >                  # reset the profile
> >                  self.runner.reset()
> > +            self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> > +                                                 self.web_server_url})
>
> Again, why does mozprofile not set this pref automatically when the profile
> gets reset? It should usually restore all the pref it knows about.
TODO

> @@ +377,5 @@
> > +            # set docroot
> > +            if not self.server_root and 'server-root' in test:
> > +                self.httpd.router.doc_root = os.path.abspath(
> > +                    os.path.join(test['path'].split(test['relpath'])[0],
> > +                                 test['server-root']))
>
> I would construct the path in its own line to improve readability here.
Done

> @@ +604,5 @@
> > +        # https://github.com/w3c/wptserve/pull/31
> > +        content_types = wptserve.utils.invert_dict({
> > +            'application/json': ['json'],
> > +            'application/x-xpinstall': ['xpi'],})
> > +        wptserve.constants.content_types.update(content_types)
>
> You know that I'm not a fan of munging with private/protected class members.
> This is one more instance of it, which I don't see as solved correctly.
>
> Why can't we use a __dir__.headers file for folders with files of to be
> added content types, as described at the following page. It would be similar
> to a .htaccess file for Apache.
>
> http://wptserve.readthedocs.org/en/latest/handlers.html#file-handlers
We'll wait for a new wptserve version to come out

> @@ +607,5 @@
> > +            'application/x-xpinstall': ['xpi'],})
> > +        wptserve.constants.content_types.update(content_types)
> > +
> > +        # start the server
> > +        self.server_root = server_root
>
> This assignment has to take place in the constructor, and should not be
> delegated to a class method.
Right, done.

> @@ +611,5 @@
> > +        self.server_root = server_root
> > +        port = WEB_SERVER_PORT
> > +        while True:
> > +            try:
> > +                self.httpd = wptserve.server.WebTestHttpd(
>
> lets call it self.http_server. The 'd' suffix would mean it is a daemon,
> which indeed it is not.
Done

> @@ +620,5 @@
> > +                if self.httpd.get_url():
> > +                    break
> > +            except:
> > +                port += 1
> > +        self.web_server_url = self.httpd.get_url()
>
> Is there a reason we have to store it? We could simply call get_url() when
> it is necessary.
Nope, you asked to have it stored in a previous review.

> @@ +622,5 @@
> > +            except:
> > +                port += 1
> > +        self.web_server_url = self.httpd.get_url()
> > +
> > +        # expose the url as a pref
>
> nit: URL
Done

> @@ +624,5 @@
> > +        self.web_server_url = self.httpd.get_url()
> > +
> > +        # expose the url as a pref
> > +        self.runner.profile.set_preferences({'extensions.mozmill.baseurl':
> > +                                             self.web_server_url})
>
> Still not sure if this is the right way to set default preferences. As you
> have seen it get lost after a reset. So please check if we can set it so it
> sticks.

TODO

> @@ +687,5 @@
> >          self.manifest = TestManifest(manifests=self.options.manifests,
> >                                       strict=False)
> >
> > +        # get server_root from cli
> > +        self.server_root = self.options.server_root or None
>
> This line can be removed. It's not necessary. You already specify None as
> default in optparse.
Done

> @@ +788,5 @@
> >                           help='Path of directory to use for screenshots')
> > +        group.add_option('--server-root',
> > +                         dest='server_root',
> > +                         default=None,
> > +                         help="Document root for serving local content")
>
> nit: I would say 'serving local testcases'. Also keep to single quotes.
Done

> @@ +859,5 @@
> >          mozmill = MozMill(runner, self.jsbridge_port,
> >                            jsbridge_timeout=self.options.timeout,
> >                            handlers=self.event_handlers,
> > +                          screenshots_path=self.options.screenshots_path,
> > +                          server_root=self.server_root)
>
> `=self.options.server_root`
Done

> ::: mozmill/mozmill/extension/resource/driver/controller.js
> @@ +502,5 @@
> >    return true;
> >  }
> >
> > +let logDeprecated = utils.logDeprecated;
> > +let logDeprecatedAssert = utils.logDeprecatedAssert;
>
> Instead of defining it could you use utils.logDeprecated directly for any
> instance of that file? Same for the assert.
Done

> ::: mozmill/mozmill/extension/resource/modules/frame.js
> @@ +33,5 @@
> >  var mozmill = undefined;
> >  var mozelement = undefined;
> >  var modules = undefined;
> >
> > +var baseurl = utils.getPreference('extensions.mozmill.baseurl', '');
>
> I don't see why this needs to be global. Make it a property of Collector, or
> retrieve the value in its constructor.
Done.

> @@ +450,5 @@
> >    this.testing = [];
> >  }
> >
> >  Collector.prototype.addHttpResource = function collector_addHttpResource(aDirectory, aPath) {
> > +  utils.logDeprecated('Collector.addHttpResource', 'Define the root in the manifest file');
>
> nit: 'Define the base_url as server_root...'
Done

> ::: mutt/mutt/tests/js/testController/manifest.ini
> @@ -1,2 @@
> > -[DEFAULT]
> > -type = javascript
>
> Why do you remove that? It's necessary for Mutt to determine the type of
> test.
This gets inherited from `mutt/mutt/tests/js/manifest.ini` across all JS files.
But yeah, maybe this shouldn't be part of this patch. (I intially included this
because of our dependency to also land parent:includes).
I've reverted them.

> ::: mutt/mutt/tests/js/testFrame/testHttpd/manifest.ini
> @@ -1,5 @@
> > -[DEFAULT]
> > -type = javascript
> > -
> > -[testMultipleLoads.js]
> > -[testRestartAvailability.js]
>
> The functionality of the http server is provided via frames.js, so please
> keep the tests in this folder.
Done

Comment 39

[Andrei Eftimie]

Comment on attachment 8449500 [details] [diff] [review]
0010-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

I actually don't have other updates at the moment.

Besides the notes from the previous comment:

1) Tested on Windows and it works great!
2) Regarding cache, the problem we fail in testHttpd/testMultipleLoads/test2.js is that wptserve doesn't set the "Last-Modified" header on the default filehandler which we use.
I've issued a pr to fix this in wptserve https://github.com/w3c/wptserve/pull/35, but I've talked to jgraham on IRC and this is a feature, to preserve as little state as possible between tests.
If we'll want to have cached content in mozmill, we'll have to either extend or copy the filehandler locally (and change what we need).
I'm not sure we need this. Local files will not change during a testrun, so serving them from cache sounds feasible for us... but the speed improvement is so small it won't matter.
I'd propose to not implement cache. If we do this, we'll need to amend the testMultipleLoads.js test to stop checking for the cached response.

3) I haven't found a way to start the server _before_ the preferences are set on the runner. If we don't to this, once the profile gets deleted (eg controller.stopApplication()) we lose the pref to 'extensions.mozmill.baseurl' so we have to re set it before starting the runner. If you have any ideas on how to proceed here, I'm ok with trying them out.
Thanks

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #39)
> 2) Regarding cache, the problem we fail in
> testHttpd/testMultipleLoads/test2.js is that wptserve doesn't set the
> "Last-Modified" header on the default filehandler which we use.
> I've issued a pr to fix this in wptserve
> https://github.com/w3c/wptserve/pull/35, but I've talked to jgraham on IRC
> and this is a feature, to preserve as little state as possible between tests.

Lets see if that gets landed. For now disable the test, and reference this PR as comment. If it doesn't get fixed we should remove this unit test, given that it was also specific to httpd.js. It's not blocking us here.

> 3) I haven't found a way to start the server _before_ the preferences are
> set on the runner. If we don't to this, once the profile gets deleted (eg
> controller.stopApplication()) we lose the pref to
> 'extensions.mozmill.baseurl' so we have to re set it before starting the
> runner. If you have any ideas on how to proceed here, I'm ok with trying
> them out.

I don't understand why you cannot use the add() method here:
http://mozbase.readthedocs.org/en/latest/mozprofile.html#mozprofile.prefs.Preferences.add

Comment 41

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #40)
> (In reply to Andrei Eftimie from comment #39)
> > 2) Regarding cache, the problem we fail in
> > testHttpd/testMultipleLoads/test2.js is that wptserve doesn't set the
> > "Last-Modified" header on the default filehandler which we use.
> > I've issued a pr to fix this in wptserve
> > https://github.com/w3c/wptserve/pull/35, but I've talked to jgraham on IRC
> > and this is a feature, to preserve as little state as possible between tests.
> 
> Lets see if that gets landed. For now disable the test, and reference this
> PR as comment. If it doesn't get fixed we should remove this unit test,
> given that it was also specific to httpd.js. It's not blocking us here.

You misread me. I talked to jgraham on irc and he won't land that because he wants to preserve as little state as possible between tests. If we want this we'll need to either extend the default filehandler in our code, or copy the whole handler (and make the required changes) into our code.

> > 3) I haven't found a way to start the server _before_ the preferences are
> > set on the runner. If we don't to this, once the profile gets deleted (eg
> > controller.stopApplication()) we lose the pref to
> > 'extensions.mozmill.baseurl' so we have to re set it before starting the
> > runner. If you have any ideas on how to proceed here, I'm ok with trying
> > them out.
> 
> I don't understand why you cannot use the add() method here:
> http://mozbase.readthedocs.org/en/latest/mozprofile.html#mozprofile.prefs.
> Preferences.add
I'll give this a try.

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8449500 [details] [diff] [review]
0010-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

Looks great and we seem to be nearly ready here. Please just address the nits and we are fine.

::: mozmill/mozmill/__init__.py
@@ +376,5 @@
>          try:
> +            # set docroot
> +            if not self.server_root and 'server-root' in test:
> +                path = test['path'].split(test['relpath'])[0]
> +                root = os.path.abspath(os.path.join(path, test['server-root']))

Just a nit: I would have done the root path construction in a single command as mentioned in my last review. You can move the abspath() call to the assignment line.

@@ +600,5 @@
> +    # init and start the http server
> +    def http_server_start(self):
> +        # Custom routes, just serve any file via GET or POST
> +        routes = [("GET", "*", wptserve.handlers.file_handler),
> +                  ("POST", "*", wptserve.handlers.file_handler),]

So by default POST requests are not handled? If that is the case, those will not be added for files? Or could we come up with a fix?

Otherwise I don't see why we have to re-define the route for GET. This is already done, and we should just add our custom root for POST.

@@ +612,5 @@
> +                    host='localhost',
> +                    port=port,
> +                    routes=routes)
> +                self.http_server.start()
> +                if self.http_server.get_url():

So just to wrap up here. If the specified port is already in use, nor __init__() and start() raise an exception?

@@ +624,5 @@
> +
> +    # stop the http server
> +    def http_server_stop(self):
> +        if self.http_server:
> +            self.http_server.stop()

Have you tested how it reacts if the server has not been started? Would that raise an exception?

::: mutt/mutt/tests/js/testUtils/testPageLoad.js
@@ +5,3 @@
>  const TEST_DATA = [
>    // Normal pages
> +  {url: baseurl + "form.html", type: "ID", value: "fname"},

So we have the restriction here that we cannot run this test alone for now. Mind adding this to the readme for Mutt?

Comment 43

[Andrei Eftimie]

Attachment: 0012-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

(In reply to Henrik Skupin (:whimboo) from comment #42)
> Comment on attachment 8449500 [details] [diff] [review]
> 0010-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch
>
> Review of attachment 8449500 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looks great and we seem to be nearly ready here. Please just address the
> nits and we are fine.
>
> ::: mozmill/mozmill/__init__.py
> @@ +376,5 @@
> >          try:
> > +            # set docroot
> > +            if not self.server_root and 'server-root' in test:
> > +                path = test['path'].split(test['relpath'])[0]
> > +                root = os.path.abspath(os.path.join(path, test['server-root']))
>
> Just a nit: I would have done the root path construction in a single command
> as mentioned in my last review. You can move the abspath() call to the
> assignment line.
Done

> @@ +600,5 @@
> > +    # init and start the http server
> > +    def http_server_start(self):
> > +        # Custom routes, just serve any file via GET or POST
> > +        routes = [("GET", "*", wptserve.handlers.file_handler),
> > +                  ("POST", "*", wptserve.handlers.file_handler),]
>
> So by default POST requests are not handled? If that is the case, those will
> not be added for files? Or could we come up with a fix?
You have to specify a list of 'routes' with which the Router is initialised.
Without it, the server will use the default one:

https://github.com/w3c/wptserve/blob/3b401bf855b8582e66d6a33f3da30ba0357530bd/wptserve/routes.py#L3-L6
> routes = [(any_method, "*.py", handlers.python_script_handler),
>           ("GET", "*.asis", handlers.as_is_handler),
>           ("GET", "*", handlers.file_handler),
>           ]

Now we don't care for Python or the "As Is" handler. We want to use the default
file handler for both GET and POST requests.

I've improved this by using the `any_method` object that handles both GET and
POST requests.

> @@ +612,5 @@
> > +                    host='localhost',
> > +                    port=port,
> > +                    routes=routes)
> > +                self.http_server.start()
> > +                if self.http_server.get_url():
>
> So just to wrap up here. If the specified port is already in use, nor
> __init__() and start() raise an exception?

If the port is already in use, we will start the server on a free (incremented)
port. I've just ran 2 full mutt tests in parallel.

> @@ +624,5 @@
> > +
> > +    # stop the http server
> > +    def http_server_stop(self):
> > +        if self.http_server:
> > +            self.http_server.stop()
>
> Have you tested how it reacts if the server has not been started? Would that
> raise an exception?

As per the stop method used:
> If the server is not running, this method has no effect.
http://wptserve.readthedocs.org/en/latest/server.html?highlight=stop#wptserve.server.WebTestHttpd.stop

> ::: mutt/mutt/tests/js/testUtils/testPageLoad.js
> @@ +5,3 @@
> >  const TEST_DATA = [
> >    // Normal pages
> > +  {url: baseurl + "form.html", type: "ID", value: "fname"},
>
> So we have the restriction here that we cannot run this test alone for now.
> Mind adding this to the readme for Mutt?
Huh? Why do you think that?
The test currently passes.

======

Plus:
- I've removed the multipleLoad test as discussed
- For the preference it seems safe to not re-set the pref after a profile reset,
I have all tests passing. Not sure why earlier I had failures when not
re-setting them. Tested it thouroughly and everything is green.

From mozprofile docs, we are using a documented way of setting preferences:
> Preferences can be set in several ways:
> - using the API: You can make a dictionary with the preferences and pass it to the Profile constructor. You can also add more preferences with the Profile.set_preferences method.
> - using a JSON blob file: mozprofile --preferences myprefs.json
> - using a .ini file: mozprofile --preferences myprefs.ini
> - via the command line: mozprofile --pref key:value --pref key:value [...]
(http://mozbase.readthedocs.org/en/latest/mozprofile.html#module-mozprofile.prefs)
We can't set them at init timer becuase we don't have the server running at that
time. We are using the first method mentioned here.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #43)
> > @@ +612,5 @@
> > > +                    host='localhost',
> > > +                    port=port,
> > > +                    routes=routes)
> > > +                self.http_server.start()
> > > +                if self.http_server.get_url():
> >
> > So just to wrap up here. If the specified port is already in use, nor
> > __init__() and start() raise an exception?
> 
> If the port is already in use, we will start the server on a free
> (incremented)
> port. I've just ran 2 full mutt tests in parallel.

This does not answer my question. From my view start() should really fail.

> > ::: mutt/mutt/tests/js/testUtils/testPageLoad.js
> > @@ +5,3 @@
> > >  const TEST_DATA = [
> > >    // Normal pages
> > > +  {url: baseurl + "form.html", type: "ID", value: "fname"},
> >
> > So we have the restriction here that we cannot run this test alone for now.
> > Mind adding this to the readme for Mutt?
> Huh? Why do you think that?
> The test currently passes.

So you can run `mutt testjs -m mutt/mutt/tests/js/testUtils/manifest.ini without problems?

Comment 45

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #44)
> (In reply to Andrei Eftimie from comment #43)
> > > @@ +612,5 @@
> > > > +                    host='localhost',
> > > > +                    port=port,
> > > > +                    routes=routes)
> > > > +                self.http_server.start()
> > > > +                if self.http_server.get_url():
> > >
> > > So just to wrap up here. If the specified port is already in use, nor
> > > __init__() and start() raise an exception?
> > 
> > If the port is already in use, we will start the server on a free
> > (incremented)
> > port. I've just ran 2 full mutt tests in parallel.
> 
> This does not answer my question. From my view start() should really fail.

That's why we're in a try/except block... 
We get `socket.error: [Errno 48] Address already in use` in except we increment the port then we try again. I think the code is pretty self-explanatory. Do you feel we should also add a comment?

> > > ::: mutt/mutt/tests/js/testUtils/testPageLoad.js
> > > @@ +5,3 @@
> > > >  const TEST_DATA = [
> > > >    // Normal pages
> > > > +  {url: baseurl + "form.html", type: "ID", value: "fname"},
> > >
> > > So we have the restriction here that we cannot run this test alone for now.
> > > Mind adding this to the readme for Mutt?
> > Huh? Why do you think that?
> > The test currently passes.
> 
> So you can run `mutt testjs -m mutt/mutt/tests/js/testUtils/manifest.ini
> without problems?

Yes, the command would be:
`mozmill -m mutt/mutt/tests/js/testUtils/manifest.ini -b /Applications/FirefoxNightly.app/ --server-root=mutt/mutt/tests/data/`

Comment 46

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #44)
> (In reply to Andrei Eftimie from comment #43)
> > > @@ +612,5 @@
> > > > +                    host='localhost',
> > > > +                    port=port,
> > > > +                    routes=routes)
> > > > +                self.http_server.start()
> > > > +                if self.http_server.get_url():
> > >
> > > So just to wrap up here. If the specified port is already in use, nor
> > > __init__() and start() raise an exception?
> > 
> > If the port is already in use, we will start the server on a free
> > (incremented)
> > port. I've just ran 2 full mutt tests in parallel.
> 
> This does not answer my question. From my view start() should really fail.

That's why we're in a try/except block... 
We get `socket.error: [Errno 48] Address already in use` in except we increment the port then we try again. I think the code is pretty self-explanatory. Do you feel we should also add a comment?

> > > ::: mutt/mutt/tests/js/testUtils/testPageLoad.js
> > > @@ +5,3 @@
> > > >  const TEST_DATA = [
> > > >    // Normal pages
> > > > +  {url: baseurl + "form.html", type: "ID", value: "fname"},
> > >
> > > So we have the restriction here that we cannot run this test alone for now.
> > > Mind adding this to the readme for Mutt?
> > Huh? Why do you think that?
> > The test currently passes.
> 
> So you can run `mutt testjs -m mutt/mutt/tests/js/testUtils/manifest.ini
> without problems?

Yes, the command would be:
`mozmill -m mutt/mutt/tests/js/testUtils/manifest.ini -b /Applications/FirefoxNightly.app/ --server-root=mutt/mutt/tests/data/`

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #45)
> That's why we're in a try/except block... 
> We get `socket.error: [Errno 48] Address already in use` in except we
> increment the port then we try again. I think the code is pretty
> self-explanatory. Do you feel we should also add a comment?

Well, then the following if condition is not necessary. Please remove it:

+                if self.http_server.get_url():
+                    break

> > > > ::: mutt/mutt/tests/js/testUtils/testPageLoad.js
> > > > @@ +5,3 @@
> > > > >  const TEST_DATA = [
> > > > >    // Normal pages
> > > > > +  {url: baseurl + "form.html", type: "ID", value: "fname"},
> > > >
> > > > So we have the restriction here that we cannot run this test alone for now.
> > > > Mind adding this to the readme for Mutt?
> > > Huh? Why do you think that?
> > > The test currently passes.
> > 
> > So you can run `mutt testjs -m mutt/mutt/tests/js/testUtils/manifest.ini
> > without problems?
> 
> Yes, the command would be:
> `mozmill -m mutt/mutt/tests/js/testUtils/manifest.ini -b
> /Applications/FirefoxNightly.app/ --server-root=mutt/mutt/tests/data/`

You do not want to force --server-root via the command line here. That is optional, and the tests should work without it.

Comment 48

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #47)
> (In reply to Andrei Eftimie from comment #45)
> > That's why we're in a try/except block... 
> > We get `socket.error: [Errno 48] Address already in use` in except we
> > increment the port then we try again. I think the code is pretty
> > self-explanatory. Do you feel we should also add a comment?
> 
> Well, then the following if condition is not necessary. Please remove it:
> 
> +                if self.http_server.get_url():
> +                    break

We will never break out of `while True` without this...

> > > > > ::: mutt/mutt/tests/js/testUtils/testPageLoad.js
> > > > > @@ +5,3 @@
> > > > > >  const TEST_DATA = [
> > > > > >    // Normal pages
> > > > > > +  {url: baseurl + "form.html", type: "ID", value: "fname"},
> > > > >
> > > > > So we have the restriction here that we cannot run this test alone for now.
> > > > > Mind adding this to the readme for Mutt?
> > > > Huh? Why do you think that?
> > > > The test currently passes.
> > > 
> > > So you can run `mutt testjs -m mutt/mutt/tests/js/testUtils/manifest.ini
> > > without problems?
> > 
> > Yes, the command would be:
> > `mozmill -m mutt/mutt/tests/js/testUtils/manifest.ini -b
> > /Applications/FirefoxNightly.app/ --server-root=mutt/mutt/tests/data/`
> 
> You do not want to force --server-root via the command line here. That is
> optional, and the tests should work without it.

You can't deliver files if you don't know the server root.
This will only work without `--server-root` as a cli argument after bug 1023790 also lands.

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #48)
> > +                if self.http_server.get_url():
> > +                    break
> 
> We will never break out of `while True` without this...

Why not? I don't say that you should remove the `break`.

> > > Yes, the command would be:
> > > `mozmill -m mutt/mutt/tests/js/testUtils/manifest.ini -b
> > > /Applications/FirefoxNightly.app/ --server-root=mutt/mutt/tests/data/`
> > 
> > You do not want to force --server-root via the command line here. That is
> > optional, and the tests should work without it.
> 
> You can't deliver files if you don't know the server root.
> This will only work without `--server-root` as a cli argument after bug
> 1023790 also lands.

This bug is totally unrelated. What has to be done here is that you want to retrieve the base url via the manifest. I don't see that any of the manifest files have been updated yet.

Comment 50

[Andrei Eftimie]

Attachment: 0013-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

Fixed the nits and added server-root to all mutt manifest files.
Works with both relative and absolute paths.

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8452382 [details] [diff] [review]
0013-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

::: mozmill/mozmill/__init__.py
@@ +372,5 @@
>  
>          """
>          try:
> +            # set docroot
> +            if not self.server_root and 'server-root' in test:

Would you mind to move this whole block into it's own method? I would like to see some comments here what those if conditions are. Otherwise it will be very hard to remember later, what this is actually doing.

@@ +377,5 @@
> +                if os.path.isabs(test['server-root']):
> +                    root = test['server-root']
> +                else:
> +                    root = os.path.abspath(
> +                        os.path.join(test['here'], test['server-root']))

I would split the join() call into two lines, which makes it better readable.

@@ +601,5 @@
> +    # init and start the http server
> +    def http_server_start(self):
> +        # Custom routes, just serve any file via GET or POST
> +        routes = [
> +            (wptserve.router.any_method, "*", wptserve.handlers.file_handler)]

Can we really please stop mangling with internal stuff of wptserve for our purposes? I mentioned that a couple of times meanwhile, and each time another method is used. No, we don't want to use that. Please be more thoughtfully in investigation possible solutions.

So I wondered about server_instance.router.register_handler(). Sadly router not documented as public property. So I talked with jgraham on IRC (what you should have done yourself) about that, and we can indeed use server_instance.router.register_handler() for adding new handlers. He will add that part to the docs in a bit.

@@ +612,5 @@
> +                    doc_root=self.server_root,
> +                    host='localhost',
> +                    port=port,
> +                    routes=routes)
> +                self.http_server.start()

So I finally tested this bit, and I really wonder why we need all that? You said multiple times that wptserve is not able to find a free port. That is not true. It works wonderful when passing `port=0` into the constructor.

So can you please elaborate on that?

::: mutt/mutt/tests/python/test_multiple_run.py
@@ +18,3 @@
>          tests = [{'path': testpath}]
>  
> +        m = mozmill.MozMill.create(server_root=os.path.join(here, '../data'))

We should get the root added to a manifest for this Mozmill JS test. Using server_root as argument should be a separate test. But this is basic.

Comment 52

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #51)
> @@ +601,5 @@
> > +    # init and start the http server
> > +    def http_server_start(self):
> > +        # Custom routes, just serve any file via GET or POST
> > +        routes = [
> > +            (wptserve.router.any_method, "*", wptserve.handlers.file_handler)]
> 
> Can we really please stop mangling with internal stuff of wptserve for our
> purposes? I mentioned that a couple of times meanwhile, and each time
> another method is used. No, we don't want to use that. Please be more
> thoughtfully in investigation possible solutions.
> 
> So I wondered about server_instance.router.register_handler(). Sadly router
> not documented as public property. So I talked with jgraham on IRC (what you
> should have done yourself) about that, and we can indeed use
> server_instance.router.register_handler() for adding new handlers. He will
> add that part to the docs in a bit.

Why do you say "mangling internal stuff of wptserve"?
Documentation on how to instantiate a server:
http://wptserve.readthedocs.org/en/latest/server.html#module-wptserve.server

Please note the `routes` argument:
> routes – List of routes with which to initialize the router
What I've done here is to create a routes object with the routes that we want to handle, and passed in the default wptserve filehandler.

Please also note right above how the docs show an example of instantiating a server:
> httpd = server.WebTestHttpd(port=8080, doc_root="./files/",
>                             routes=[("GET", "*", handlers.file_handler)])

I'm not sure what you want. Instead of passing in the routes at init time, we should first create the instance then call register_handler() at a later time to pass in the filehandler?

I don't follow what this would accomplish...

Comment 53

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #52)
> I'm not sure what you want. Instead of passing in the routes at init time,
> we should first create the instance then call register_handler() at a later
> time to pass in the filehandler?

You correctly read my statement. So that is exactly what we have to use here.

Comment 54

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #53)
> (In reply to Andrei Eftimie from comment #52)
> > I'm not sure what you want. Instead of passing in the routes at init time,
> > we should first create the instance then call register_handler() at a later
> > time to pass in the filehandler?
> 
> You correctly read my statement. So that is exactly what we have to use here.

You still haven't answered my question.

==

So what do we gain for setting the routes after init?

> http_server = wptserve.server.WebTestHttpd(
>     doc_root=self.server_root,
>     host='localhost',
>     port=0,
>     routes=[(wptserve.router.any_method,
>              "*", wptserve.handlers.file_handler)])

vs:

> http_server = wptserve.server.WebTestHttpd(
>     doc_root=self.server_root,
>     host='localhost',
>     port=0,
>     routes=[])
> 
> http_server.router.register(wptserve.router.any_method,
>                             "*", wptserve.handlers.file_handler)

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #54)
> So what do we gain for setting the routes after init?

We only gain a better readability here. It may be my preference to try to limit customizations for __init__, and keep them for later. It's easy to excess the constructor, as we can see in various mozbase packages. So I wont force this change, and if you want leave it as it is.

> >     routes=[(wptserve.router.any_method,
> >              "*", wptserve.handlers.file_handler)])
> 
> vs:
> 
> > http_server.router.register(wptserve.router.any_method,
> >                             "*", wptserve.handlers.file_handler)

But whatever method we use please don't use `wptserve.router.any_method`. This is not mentioned as public and even the docs say to use '*' here. So please do so, and keep the single quotes.

Comment 56

[Andrei Eftimie]

Attachment: 0014-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

(In reply to Henrik Skupin (:whimboo) from comment #55)
> (In reply to Andrei Eftimie from comment #54)
> > So what do we gain for setting the routes after init?
> 
> We only gain a better readability here. It may be my preference to try to
> limit customizations for __init__, and keep them for later. It's easy to
> excess the constructor, as we can see in various mozbase packages. So I wont
> force this change, and if you want leave it as it is.

You could have simply asked and stated this at the beginning instead of throwing "mangling internal stuff" around which we are not doing. I don't have a strong personal preference for this. I switched to `router.register`

> > >     routes=[(wptserve.router.any_method,
> > >              "*", wptserve.handlers.file_handler)])
> > 
> > vs:
> > 
> > > http_server.router.register(wptserve.router.any_method,
> > >                             "*", wptserve.handlers.file_handler)
> 
> But whatever method we use please don't use `wptserve.router.any_method`.
> This is not mentioned as public and even the docs say to use '*' here. So
> please do so, and keep the single quotes.

From the docs: http://wptserve.readthedocs.org/en/latest/router.html#router
> methods is either a string or a list of strings indicating the HTTP methods
> to match. In cases where all methods should match there is a special sentinel
> value any_method provided as a property of the router module that can be used.
The last part says "that can be used".

What you were saying is that we shouldn't use `router.any_method` because `router` is not public. But instead of passing in the routes at init we should use `router.register`, which uses the same "non-public" `router` object.

Also we can't use '*' as a method (see the previous quoted doc). We need to pass in a string or a list of strings which matches the methods. I've updated the route to pass in both GET and POST together. 

(In reply to Henrik Skupin (:whimboo) from comment #51)
> Comment on attachment 8452382 [details] [diff] [review]
> 0013-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch
> 
> Review of attachment 8452382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozmill/mozmill/__init__.py
> @@ +372,5 @@
> >  
> >          """
> >          try:
> > +            # set docroot
> > +            if not self.server_root and 'server-root' in test:
> 
> Would you mind to move this whole block into it's own method? I would like
> to see some comments here what those if conditions are. Otherwise it will be
> very hard to remember later, what this is actually doing.
Done

> @@ +377,5 @@
> > +                if os.path.isabs(test['server-root']):
> > +                    root = test['server-root']
> > +                else:
> > +                    root = os.path.abspath(
> > +                        os.path.join(test['here'], test['server-root']))
> 
> I would split the join() call into two lines, which makes it better readable.
Done

> So I finally tested this bit, and I really wonder why we need all that? You
> said multiple times that wptserve is not able to find a free port. That is
> not true. It works wonderful when passing `port=0` into the constructor.
> 
> So can you please elaborate on that?
Really nice find indeed. This only works with `0`. Not specifying a port at all 
or setting any other port than 0 fail miserably. With `0` indeed wptserve
managesto find a free port. I've updated the code accrodingly.

This is not documented behaviour nor obvious in it's source code (as far as I
was able to determine).
Thanks.

> ::: mutt/mutt/tests/python/test_multiple_run.py
> @@ +18,3 @@
> >          tests = [{'path': testpath}]
> >  
> > +        m = mozmill.MozMill.create(server_root=os.path.join(here, '../data'))
> 
> We should get the root added to a manifest for this Mozmill JS test. Using
> server_root as argument should be a separate test. But this is basic.
I've changed this test to use a manifest file with the server-root set up.
It works fine in this case, but for other mutt python tests where we call the 
test directly we still need to instantiate Mozmill directly with the server-root

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #56)
> From the docs: http://wptserve.readthedocs.org/en/latest/router.html#router
> > methods is either a string or a list of strings indicating the HTTP methods
> > to match. In cases where all methods should match there is a special sentinel
> > value any_method provided as a property of the router module that can be used.
> The last part says "that can be used".

That's something I have not seen, given that I was looking for the register method:
http://wptserve.readthedocs.org/en/latest/router.html#wptserve.router.Router.register

> What you were saying is that we shouldn't use `router.any_method` because
> `router` is not public. But instead of passing in the routes at init we
> should use `router.register`, which uses the same "non-public" `router`
> object.

As I clarified two reviews back, I spoke with jgraham and he fixed that in the docs. router is a public property, which can be used.

> Also we can't use '*' as a method (see the previous quoted doc). We need to
> pass in a string or a list of strings which matches the methods. I've
> updated the route to pass in both GET and POST together. 

This conflicts with the documentation I pasted above.

> methods – Set of methods this should match. “*” is a special value indicating that all methods should be matched.

Please clarify that.

> or setting any other port than 0 fail miserably. With `0` indeed wptserve
> managesto find a free port. I've updated the code accrodingly.
> 
> This is not documented behaviour nor obvious in it's source code (as far as I
> was able to determine).
> Thanks.

That may be known by a lot of Python devs, who worked with networking stuff before. But in case others want to use it, I would suggest to ping jgraham to fix the docs here.

> > ::: mutt/mutt/tests/python/test_multiple_run.py
> > @@ +18,3 @@
> > >          tests = [{'path': testpath}]
> > >  
> > > +        m = mozmill.MozMill.create(server_root=os.path.join(here, '../data'))
> > 
> > We should get the root added to a manifest for this Mozmill JS test. Using
> > server_root as argument should be a separate test. But this is basic.
> I've changed this test to use a manifest file with the server-root set up.
> It works fine in this case, but for other mutt python tests where we call
> the 
> test directly we still need to instantiate Mozmill directly with the
> server-root

Do those tests use the base_url property? If not I don't see why we would have to do it.

Comment 58

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #57)
> (In reply to Andrei Eftimie from comment #56)
> > From the docs: http://wptserve.readthedocs.org/en/latest/router.html#router
> > > methods is either a string or a list of strings indicating the HTTP methods
> > > to match. In cases where all methods should match there is a special sentinel
> > > value any_method provided as a property of the router module that can be used.
> > The last part says "that can be used".
> 
> That's something I have not seen, given that I was looking for the register
> method:
> http://wptserve.readthedocs.org/en/latest/router.html#wptserve.router.Router.
> register
> > Also we can't use '*' as a method (see the previous quoted doc). We need to
> > pass in a string or a list of strings which matches the methods. I've
> > updated the route to pass in both GET and POST together. 
> 
> This conflicts with the documentation I pasted above.
> > methods – Set of methods this should match. “*” is a special value indicating that all methods should be matched.

Interesting. The docs do conflict here.
I did test '*' and it failed. 
I'll have another look to make sure.
 
> Please clarify that.
> 
> > or setting any other port than 0 fail miserably. With `0` indeed wptserve
> > managesto find a free port. I've updated the code accrodingly.
> > 
> > This is not documented behaviour nor obvious in it's source code (as far as I
> > was able to determine).
> > Thanks.
> 
> That may be known by a lot of Python devs, who worked with networking stuff
> before. But in case others want to use it, I would suggest to ping jgraham
> to fix the docs here.
> 
> > > ::: mutt/mutt/tests/python/test_multiple_run.py
> > > @@ +18,3 @@
> > > >          tests = [{'path': testpath}]
> > > >  
> > > > +        m = mozmill.MozMill.create(server_root=os.path.join(here, '../data'))
> > > 
> > > We should get the root added to a manifest for this Mozmill JS test. Using
> > > server_root as argument should be a separate test. But this is basic.
> > I've changed this test to use a manifest file with the server-root set up.
> > It works fine in this case, but for other mutt python tests where we call
> > the 
> > test directly we still need to instantiate Mozmill directly with the
> > server-root
> 
> Do those tests use the base_url property? If not I don't see why we would
> have to do it.

I've only updated the tests which would fail, or which would try loading a local page (which failed to load) but the tests themselves still passed.
I'll take another look at them.

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8453671 [details] [diff] [review]
0014-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

::: mozmill/mozmill/__init__.py
@@ +605,5 @@
> +            root = test['server-root']
> +        else:
> +            root = os.path.abspath(
> +                os.path.join(test['here'],
> +                             test['server-root']))

I haven't said to use 3 lines for setting the path here. Why does os.path.join not fit into the first line?

@@ +613,5 @@
> +    def http_server_start(self):
> +        # start the server
> +        while True:
> +            try:
> +                self.http_server = wptserve.server.WebTestHttpd(

I don't see why we still need the while loop here. We will get a free port to use, if not it will never succeed.

@@ +617,5 @@
> +                self.http_server = wptserve.server.WebTestHttpd(
> +                    doc_root=self.server_root,
> +                    host='localhost',
> +                    port=0,
> +                    routes=[])

Get rid of the empty routes array here. routes has a default value so no need to specify it anymore.

@@ +625,5 @@
> +                pass
> +
> +        # Custom routes, just serve any file via GET or POST
> +        self.http_server.router.register(
> +            ['GET', 'POST'], '*', wptserve.handlers.file_handler)

I think that we do not necessarily need the '*' value here as first parameter. Given that with the removal of routes in the constructor the GET handler is already set, and we only have to register POST.

::: mutt/mutt/tests/python/test_page_load.py
@@ +16,3 @@
>          tests = [{'path': testpath}]
>  
> +        m = mozmill.MozMill.create(server_root=os.path.join(here, '../data'))

As said in my last review please use in all tests a manifest file. Also I miss the requested CLI test for this option.

Comment 60

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) [away 07/19 - 08/01] from comment #59) 
> @@ +617,5 @@
> > +                self.http_server = wptserve.server.WebTestHttpd(
> > +                    doc_root=self.server_root,
> > +                    host='localhost',
> > +                    port=0,
> > +                    routes=[])
> 
> Get rid of the empty routes array here. routes has a default value so no
> need to specify it anymore.
The default routes list has 3 entries (2 of which we do not need).
ATM we don't serve ".py" or ".asis" files, we shouldn't include them if they are
not needed and not used. That's one of the reasons I wanted to pass in the
routes at init time.

> @@ +625,5 @@
> > +                pass
> > +
> > +        # Custom routes, just serve any file via GET or POST
> > +        self.http_server.router.register(
> > +            ['GET', 'POST'], '*', wptserve.handlers.file_handler)
> 
> I think that we do not necessarily need the '*' value here as first
> parameter. Given that with the removal of routes in the constructor the GET
> handler is already set, and we only have to register POST.
See above. We would be left with 4 routes, 2 out of which we don't use / need.

Comment 61

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #60)
> > Get rid of the empty routes array here. routes has a default value so no
> > need to specify it anymore.
> The default routes list has 3 entries (2 of which we do not need).
> ATM we don't serve ".py" or ".asis" files, we shouldn't include them if they
> are
> not needed and not used. That's one of the reasons I wanted to pass in the
> routes at init time.

Then you should explain that too. None of the former comments mentioned that. And you may see know why it caused confusion. Anyway, it's better to base on the defaults and add our custom routes.

Comment 62

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) [away 07/19 - 08/01] from comment #61)
> (In reply to Andrei Eftimie from comment #60)
> > > Get rid of the empty routes array here. routes has a default value so no
> > > need to specify it anymore.
> > The default routes list has 3 entries (2 of which we do not need).
> > ATM we don't serve ".py" or ".asis" files, we shouldn't include them if they
> > are
> > not needed and not used. That's one of the reasons I wanted to pass in the
> > routes at init time.
> 
> Then you should explain that too. None of the former comments mentioned
> that. And you may see know why it caused confusion. Anyway, it's better to
> base on the defaults and add our custom routes.

I did, see comment 43:
> > So by default POST requests are not handled? If that is the case, those will
> > not be added for files? Or could we come up with a fix?
> You have to specify a list of 'routes' with which the Router is initialised.
> Without it, the server will use the default one:
> 
> https://github.com/w3c/wptserve/blob/3b401bf855b8582e66d6a33f3da30ba0357530bd/wptserve/routes.py#L3-L6
> > routes = [(any_method, "*.py", handlers.python_script_handler),
> >           ("GET", "*.asis", handlers.as_is_handler),
> >           ("GET", "*", handlers.file_handler),
> >           ]
> 
> Now we don't care for Python or the "As Is" handler. We want to use the default
> file handler for both GET and POST requests.
> 
> I've improved this by using the `any_method` object that handles both GET and
> POST requests.

===

I've even given an example on how I was going to implement them in comment 54.

I'll leave the default ones in since you seem to really want that, even though we won't use them.

Comment 63

[Andrei Eftimie]

Attachment: 0015-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

(In reply to Henrik Skupin (:whimboo) [away 07/19 - 08/01] from comment #59)
> Comment on attachment 8453671 [details] [diff] [review]
> 0014-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch
>
> Review of attachment 8453671 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mozmill/mozmill/__init__.py
> @@ +605,5 @@
> > +            root = test['server-root']
> > +        else:
> > +            root = os.path.abspath(
> > +                os.path.join(test['here'],
> > +                             test['server-root']))
>
> I haven't said to use 3 lines for setting the path here. Why does
> os.path.join not fit into the first line?

You said:
> > +                    root = os.path.abspath(
> > +                        os.path.join(test['here'], test['server-root']))
> I would split the join() call into two lines, which makes it better readable.
Which is _exactly_ what I have done :/
I've updated the code now, hope it will be to your liking.

Since this is the 4th time we're changing how this is constructed, it would be
much more benefical to also propose a solution especially for stilistic changes.
This would avoid much of the back and forth we're having and save much time.

> @@ +613,5 @@
> > +    def http_server_start(self):
> > +        # start the server
> > +        while True:
> > +            try:
> > +                self.http_server = wptserve.server.WebTestHttpd(
>
> I don't see why we still need the while loop here. We will get a free port
> to use, if not it will never succeed.
Seems to work fine from my tests. Removed the while & try combo.

> ::: mutt/mutt/tests/python/test_page_load.py
> @@ +16,3 @@
> >          tests = [{'path': testpath}]
> >
> > +        m = mozmill.MozMill.create(server_root=os.path.join(here, '../data'))
>
> As said in my last review please use in all tests a manifest file. Also I
> miss the requested CLI test for this option.

All python tests that need a local file now use a manifest file with the
server-root option.

Also added test for the server-root cli option. I don't know how we could test
this synthetically, so I've requested a page and checked that it loaded via
testing the existance of an element on that page.

Comment 64

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Andrei Eftimie from comment #63)
> You said:
> > > +                    root = os.path.abspath(
> > > +                        os.path.join(test['here'], test['server-root']))
> > I would split the join() call into two lines, which makes it better readable.
> Which is _exactly_ what I have done :/
> I've updated the code now, hope it will be to your liking.
> 
> Since this is the 4th time we're changing how this is constructed, it would
> be
> much more benefical to also propose a solution especially for stilistic
> changes.
> This would avoid much of the back and forth we're having and save much time.

Something which would help way more is communication. Nothing speaks against a quick question on IRC. Placing changes in an updated patch, and waiting for a review, that makes it longer. What I would like to see, and why I didn't give proposals, is to see how you are working.

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8455296 [details] [diff] [review]
0015-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

We are getting close. Looks fine so far, but we miss the negative testcase for the newly added test.

::: mozmill/mozmill/__init__.py
@@ +614,5 @@
> +        # start the server
> +        self.http_server = wptserve.server.WebTestHttpd(
> +            doc_root=self.server_root,
> +            host='localhost',
> +            port=0)

The indentation here does not apply to the coding style used in this file. Maybe you should also directly import WebTestHttpd in the header, so that the line becomes shorter.

@@ +619,5 @@
> +        self.http_server.start()
> +
> +        # Add a custom route for POST requests to the default file_handler
> +        self.http_server.router.register(
> +            ['POST'], '*', wptserve.handlers.file_handler)

Same here as above.

::: mutt/mutt/tests/python/cli/test_server-root.py
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, you can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import subprocess
> +import os

nit: ordering.

@@ +22,5 @@
> +                                  '-t', os.path.join(testdir,
> +                                                     'useMozmill',
> +                                                     'testServerRoot.js'),
> +                                  '--server-root', os.path.join(testdir,
> +                                                                '../../data'),

This does not test the negative case. You will have to add this too.

::: mutt/mutt/tests/python/test_page_load.py
@@ +14,5 @@
> +    def do_test(self, relative_manifest_path, passes=0, fails=0, skips=0):
> +        manifestpath = os.path.join(os.path.dirname(os.path.abspath(__file__)),
> +                                    relative_manifest_path)
> +        manifest = manifestparser.TestManifest(
> +            manifests=[manifestpath], strict=False)

nit: indentation

Comment 66

[Andrei Eftimie]

Attachment: 0016-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

Fixed the indentation changes as per the last review.
Please note that this breaks PEP8 conformity.

Added the negative testcase.
The test itself still checks for the webpage (and for an element on the page).
We expect it not load in this case because we don't have the server-root set.

As per the IRC discussion:
> 1) we dont fail because an exception is thrown
We do not fail, the webserver fails in delivering the requested resource because of the missing document root. It doesn't raise an expection, it sends it as a HTTP response.

> 2) we see that the file requested cannot be served in the js test
The test tries loading the page and testing the existence of an element. We assert that the test fails.

Comment 67

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8456754 [details] [diff] [review]
0016-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

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

That looks fine. Just fix the nit and I think we should be good.

When I run the tests I see a lot of those lines written to the console:

.^/(.*)$
^/(.*)\.asis$
^/(.*)\.py$
^/(.*)$

I assume this is coming from wptserve? Is there a way to stop that? Maybe we have to file an issue for wptserve.

::: mutt/mutt/tests/python/cli/test_server-root.py
@@ +15,5 @@
> +
> +
> +class TestServerRootOption(unittest.TestCase):
> +
> +    """Test the --server-root option."""

nit: why has this extra blank line been added in the last change?

Comment 68

[Andreea Matei [:AndreeaMatei]]

Attachment: 0001-Bug-754847-Implement-wptserve-as-web-server.-r-hskup.patch

Updated to remove that extra line.

Comment 69

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Asking Andrei for feedback in regards of the wptserve output as seen in comment 67.

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Landed on master:
https://github.com/mozilla/mozmill/commit/8cad8438b6f6237f449add334f97d731448baff7

Comment 72

[Andrei Eftimie]

This is already tracked in github, and has already a pr up:
https://github.com/w3c/wptserve/issues/29

Should be fixed in a subsequent release, I'll follow-up with @jgraham to get this fixed.

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Thanks Andrei!