1028856 - Prepare mozmill-tests for the new webserver

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Andrei Eftimie]

A few changes will be needed to support bug 754847.

1) We will have to add the server-root directive as a [DEFAULT] option in root-level manifest files. This will be needed in each testrun.

2) We're keeping the `collector.addHttpResource` method for backwards compatibility. I think there are a few instances where we don't include the `data` folder directly. These will probably need a small udpate as well.

Comment 1

[Andrei Eftimie]

Attachment: 1.patch

This patch is backwards compatible.
Nothing breaks.

I didn't include any [parent:] links since even if we get the option to run deep manifest files, the inherited `server-root` value is relative to the base manifest file rendering a broken path.

With this patch we're ready to use wptserve as a webserver in mozmill.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8445056 [details] [diff] [review]
1.patch

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

::: firefox/tests/remote/restartTests/testAddons_installFromFTP/test1.js
@@ +16,5 @@
>  const TIMEOUT_DOWNLOAD = 25000;
>  
>  const ADDON = [
>    {id: "[e-mail]",
> +   url: "ftp://ftp.mozqa.com/data/firefox/addons/extensions/empty.xpi?pipe=header(Content-Type,application/x-xpinstall)"}

Hm, we might need another solution on these files or just split it since it's a very long line.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1] (away 02/13 - 02/19)]

(In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> >    {id: "[e-mail]",
> > +   url: "ftp://ftp.mozqa.com/data/firefox/addons/extensions/empty.xpi?pipe=header(Content-Type,application/x-xpinstall)"}
> 
> Hm, we might need another solution on these files or just split it since
> it's a very long line.

Well, I don't understand why we need this pipe stuff at all. Can you please explain?

Comment 4

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> > >    {id: "[e-mail]",
> > > +   url: "ftp://ftp.mozqa.com/data/firefox/addons/extensions/empty.xpi?pipe=header(Content-Type,application/x-xpinstall)"}
> > 
> > Hm, we might need another solution on these files or just split it since
> > it's a very long line.
> 
> Well, I don't understand why we need this pipe stuff at all. Can you please
> explain?

In Apache we would set a mime type in /etc/httpd/conf/mime.types (or htaccess):
> application/x-xpinstall xpi

httpd.js has this build in:
http://dxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#3025-3056
which uses Firefox mime service. Here are the defined values in Firefox:
http://dxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h#68

wptserve has some ContentTypes builtin, but it doesn't handle xpi files:
https://github.com/w3c/wptserve/blob/d2fb15f4c2239cf7ba82fb0c8b1f2396a8b17b9c/wptserve/constants.py#L3-L24

Comment 5

[Andrei Eftimie]

And without a proper mime-type Firefox treats the xpi file as an unknown file and we are prompt to download it instead of Firefox trying to install the extension.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1] (away 02/13 - 02/19)]

Then fix it in wptserve, or in the data folder. But no workarounds please. So doesn't read wptserve from htaccess files? That would be the easiest fix. Also in the case above we do not have a local URL but one from ftp.m.o.

Comment 7

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Then fix it in wptserve, or in the data folder. But no workarounds please.
What would you like to do with the data folder?

Yes, adding this particular mime-type in wptserve will solve _our_ problem. But XPI and the application/x-xpinstall are a Mozilla proprietary technology which are not use anywhere else. I don't see why we would include support for it in a w3c standards developed webserver.
Especially when wpserve provides an easy-to-use mechanism to to this ourselves from our code.

> So doesn't read wptserve from htaccess files? That would be the easiest fix.
Why do you think wptserve would read htaccess files?

> Also in the case above we do not have a local URL but one from ftp.m.o.
I was overzealous and added the content type for 1 external URL. We only need this for wptserve served content.

Comment 8

[Andrei Eftimie]

Seems Apache does provide support by default for xpi files: https://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types

Comment 9

[Andrei Eftimie]

Attachment: 2.patch

I've set up a PR to add support for the xpi content type in wptserve: https://github.com/w3c/wptserve/pull/31

This will delay our fix since that is a hard dependency now. We will have to wait for a new release of wptserve (possibly to v1.0.2) if this change would get accepted.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1] (away 02/13 - 02/19)]

(In reply to Andrei Eftimie from comment #9)
> I've set up a PR to add support for the xpi content type in wptserve:
> https://github.com/w3c/wptserve/pull/31
> 
> This will delay our fix since that is a hard dependency now. We will have to
> wait for a new release of wptserve (possibly to v1.0.2) if this change would
> get accepted.

To increase the possibility to see the patch accepted, you should really have given a good explanation in the filed issue. But sadly it is blank. You cannot expect that other people, also outside of Mozilla, know what our real intent is here. So please revisit the issue and add the necessary information.

(In reply to Andrei Eftimie from comment #7)
> Especially when wpserve provides an easy-to-use mechanism to to this
> ourselves from our code.

And what should that be? I don't call the pipe param an easy-to-use mechanism here. No, I don't want to see it used in any of our tests. This is just another workaround. A fix has to come via wptserv or directly with mozmill, but nothing else.

> > So doesn't read wptserve from htaccess files? That would be the easiest fix.
> Why do you think wptserve would read htaccess files?

This was a question. Maybe there is support for it, so we could set MIME types per directory.

Comment 11

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> (In reply to Andrei Eftimie from comment #9)
> > I've set up a PR to add support for the xpi content type in wptserve:
> > https://github.com/w3c/wptserve/pull/31
> > 
> > This will delay our fix since that is a hard dependency now. We will have to
> > wait for a new release of wptserve (possibly to v1.0.2) if this change would
> > get accepted.
> 
> To increase the possibility to see the patch accepted, you should really
> have given a good explanation in the filed issue. But sadly it is blank.
And what would the explanation be? That a 3rd party consumer of the product wants to have a custom change added into the products core. That's not enough reason to add support for it. This should get core support if the maintainers think its good for the product. Without any politics involved. 

> You cannot expect that other people, also outside of Mozilla, know what our real
> intent is here. So please revisit the issue and add the necessary
> information.
I actually expect them to _not_ care. If I were a maintainer of the project I would _not_ include support for every requested MIME type. There is a simple mechanism to control the response of the webserver. A third party needing custom could could also fork it.

Here's an "official" IANA MIME type list:
http://www.iana.org/assignments/media-types/media-types.xhtml
XPI is not a standard.

> (In reply to Andrei Eftimie from comment #7)
> > Especially when wpserve provides an easy-to-use mechanism to to this
> > ourselves from our code.
> 
> And what should that be? I don't call the pipe param an easy-to-use
> mechanism here.
It's very easy-to-use. It allows us to "configure" the responses received from the webserver on-the-fly from any test directly.

So for to add a custom MIME type you herald as easier to roll out and redeploy a new version of the software... This is actually much easier then Apache where you need to restart the webserver after you change the config. Here you simply pass in the wanted response as part of the request. I don't see how much easier it can get.

> No, I don't want to see it used in any of our tests. This is
> just another workaround. A fix has to come via wptserv or directly with
> mozmill, but nothing else.
Why did you ask to change mozhttpd with wptserve if you find wptserve's functionality to be a "workaround"?

> > > So doesn't read wptserve from htaccess files? That would be the easiest fix.
> > Why do you think wptserve would read htaccess files?
> 
> This was a question. Maybe there is support for it, so we could set MIME
> types per directory.
.htaccess is an Apache config file. I'm not aware of any other webserver other than Apache that uses .htaccess files (then again, I am not a SysAdmin / DevOp so I might be wrong here).

There's no support for it in wptserve.

===

I've went to the wptserve issue and added details about where and why we want this in.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1] (away 02/13 - 02/19)]

(In reply to Andrei Eftimie from comment #11)
> > To increase the possibility to see the patch accepted, you should really
> > have given a good explanation in the filed issue. But sadly it is blank.
>
> And what would the explanation be? That a 3rd party consumer of the product
> wants to have a custom change added into the products core. That's not
> enough reason to add support for it. This should get core support if the
> maintainers think its good for the product. Without any politics involved. 

We can go back and forth here, which I don't think is a useful spent amount of time now. Lets wait how they decide. Also this was just a general comment, to get things improved. I have seen a lot of bugs/issues filed, which lack fundamental information - not by yourself alone. 

> So for to add a custom MIME type you herald as easier to roll out and
> redeploy a new version of the software... This is actually much easier then
> Apache where you need to restart the webserver after you change the config.

You don't have to restart apache for that. Just placing an .htaccess in that folder or modifying is enough. If you want to know anything else, lets talk about it somewhere else. 

> Here you simply pass in the wanted response as part of the request. I don't
> see how much easier it can get.

We do not want to bind us to this specific web server. So no, this is something which should absolutely not be part of the test. Only the harness should decide what to do, and how to return the proper MIME type.

> > No, I don't want to see it used in any of our tests. This is
> > just another workaround. A fix has to come via wptserv or directly with
> > mozmill, but nothing else.
>
> Why did you ask to change mozhttpd with wptserve if you find wptserve's
> functionality to be a "workaround"?

I don't say wptserve is a workaround. Only the technique you are trying to use above.

Comment 13

[Andrei Eftimie]

Attachment: 3.patch

This should be all we need.
We fixed the other issues we had as part of mozmill or wptserve.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8449210 [details] [diff] [review]
3.patch

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

Looks good. We want to land this now or to wait for wptserver to be sure nothing else changes?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1] (away 02/13 - 02/19)]

Comment on attachment 8449210 [details] [diff] [review]
3.patch

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

None of the sub-manifest files have the parent link set. So this change will only take affect for a full test-run. Single folders or tests will not work.

Comment 16

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 8449210 [details] [diff] [review]
> 3.patch
> 
> Review of attachment 8449210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> None of the sub-manifest files have the parent link set. So this change will
> only take affect for a full test-run. Single folders or tests will not work.

Since we'll land the manifest parser changes (specifically the parent includes) at a later date, we'll need these bindings then.

This patch is to have mozmill-tests ready for the current update for wptserve from bug 754847

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1] (away 02/13 - 02/19)]

We will not release the next Mozmill version without the manifestparser changes. So we do a complete update of the manifests, or none at the moment and simply wait for 2.1.

Comment 18

[Andrei Eftimie]

Attachment: 5.patch

With the manifestparser parent changes working in bug 1023790, we'll need to update our test manifest files to define the root once per testruns and declare all parents.

This patch will be needed for bug 970594, and we can land it prior to having all the new functionality in mozmill and its dependencies. This passes on the current 2.0.6.2 version.

Requirements for testing this with mozmill 2.1:
- mozmill master branch with bug 1043391 on top
- mozprofile from mc (we will have to release a new version)
- manifestparser from mc with bug 1023790 on top (this will also require a new version)

Comment 19

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8475869 [details] [diff] [review]
5.patch

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

This looks good. Please just update it because the Awesomebar manifest changed. Thanks.

Comment 20

[Andrei Eftimie]

Attachment: 5.1.patch

Thanks for the review.
Updated the patch to apply correctly, ran all testruns to confirm that everything is fine.

Pushed: https://hg.mozilla.org/qa/mozmill-tests/rev/87f89645ee85

Comment 21

[Andrei Eftimie]

I'll check for backports.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1] (away 02/13 - 02/19)]

We also need this for esr24, otherwise we won't be able to run any tests with Mozmill 2.1 anymore. We cannot pin a specific version of Mozmill to a job in CI.

Comment 23

[Andrei Eftimie]

Aurora is fine, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/ca0f84d05c37 (mozilla-aurora)

Comment 24

[Andrei Eftimie]

Transplanted to Beta:
https://hg.mozilla.org/qa/mozmill-tests/rev/9d2b48893d47 (mozilla-beta)

Comment 25

[Andrei Eftimie]

Release is good as well:
https://hg.mozilla.org/qa/mozmill-tests/rev/d607bed71a58 (mozilla-release)

Comment 26

[Andrei Eftimie]

ESR31 is all good as well (I had a minor conflict in one of the addons/selenium manifest files, they are not skipped on ESR31 and running ok still).

Transplanted (with minor conflict):
https://hg.mozilla.org/qa/mozmill-tests/rev/0afa1489e922 (mozilla-esr31)

Comment 27

[Andrei Eftimie]

Attachment: esr24.patch

Had several conflicts on ESR24.
Tests are running ATM

Comment 28

[Andrei Eftimie]

All tests are good on ESR24 as well.

Comment 29

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8481301 [details] [diff] [review]
esr24.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/5e26a08f01fa (esr24)