Closed Bug 1028856 Opened 7 years ago Closed 7 years ago

Prepare mozmill-tests for the new webserver

Categories

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

defect

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed, firefox34 fixed, firefox-esr24 fixed, firefox-esr31 fixed)

RESOLVED FIXED
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed
firefox34 --- fixed
firefox-esr24 --- fixed
firefox-esr31 --- fixed

People

(Reporter: andrei, Assigned: andrei)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch 1.patch (obsolete) — Splinter Review
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.
Attachment #8445056 - Flags: review?(andreea.matei)
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
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: "test-empty@quality.mozilla.org",
> +   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.
Attachment #8445056 - Flags: review?(andreea.matei) → review-
(In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> >    {id: "test-empty@quality.mozilla.org",
> > +   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 reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> > >    {id: "test-empty@quality.mozilla.org",
> > > +   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
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.
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.
(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.
Seems Apache does provide support by default for xpi files: https://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types
Attached patch 2.patch (obsolete) — Splinter Review
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.
Attachment #8445056 - Attachment is obsolete: true
Attachment #8445726 - Flags: review?(andreea.matei)
(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.
(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.
(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.
Attached patch 3.patch (obsolete) — Splinter Review
This should be all we need.
We fixed the other issues we had as part of mozmill or wptserve.
Attachment #8445726 - Attachment is obsolete: true
Attachment #8445726 - Flags: review?(andreea.matei)
Attachment #8449210 - Flags: review?(andreea.matei)
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?
Attachment #8449210 - Flags: review?(hskupin)
Attachment #8449210 - Flags: review?(andreea.matei)
Attachment #8449210 - Flags: review+
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.
Attachment #8449210 - Flags: review?(hskupin) → review-
(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
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.
Depends on: 1023790
Attached patch 5.patch (obsolete) — Splinter Review
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)
Attachment #8475869 - Flags: review?(andreea.matei)
Blocks: 970594
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.
Attachment #8475869 - Flags: review?(andreea.matei) → review+
Attached patch 5.1.patchSplinter Review
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
Attachment #8449210 - Attachment is obsolete: true
Attachment #8475869 - Attachment is obsolete: true
Attachment #8478894 - Flags: checkin+
I'll check for backports.
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.
Aurora is fine, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/ca0f84d05c37 (mozilla-aurora)
Release is good as well:
https://hg.mozilla.org/qa/mozmill-tests/rev/d607bed71a58 (mozilla-release)
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)
Attached patch esr24.patchSplinter Review
Had several conflicts on ESR24.
Tests are running ATM
Attachment #8481301 - Flags: review?(andreea.matei)
All tests are good on ESR24 as well.
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)
Attachment #8481301 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.