Closed
Bug 1028856
Opened 9 years ago
Closed 9 years ago
Prepare mozmill-tests for the new webserver
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(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)
40.07 KB,
patch
|
andrei
:
checkin+
|
Details | Diff | Splinter Review |
37.89 KB,
patch
|
AndreeaMatei
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
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?
Assignee | ||
Comment 4•9 years ago
|
||
(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
Assignee | ||
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
Seems Apache does provide support by default for xpi files: https://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types
Assignee | ||
Comment 9•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
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 14•9 years ago
|
||
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-
Assignee | ||
Comment 16•9 years ago
|
||
(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.
Assignee | ||
Comment 18•9 years ago
|
||
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)
Comment 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
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+
Assignee | ||
Comment 21•9 years ago
|
||
I'll check for backports.
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → fixed
status-firefox-esr24:
--- → wontfix
status-firefox-esr31:
--- → affected
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.
Assignee | ||
Comment 23•9 years ago
|
||
Aurora is fine, transplanted: https://hg.mozilla.org/qa/mozmill-tests/rev/ca0f84d05c37 (mozilla-aurora)
Assignee | ||
Comment 24•9 years ago
|
||
Transplanted to Beta: https://hg.mozilla.org/qa/mozmill-tests/rev/9d2b48893d47 (mozilla-beta)
Assignee | ||
Comment 25•9 years ago
|
||
Release is good as well: https://hg.mozilla.org/qa/mozmill-tests/rev/d607bed71a58 (mozilla-release)
Assignee | ||
Comment 26•9 years ago
|
||
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)
Assignee | ||
Comment 27•9 years ago
|
||
Had several conflicts on ESR24. Tests are running ATM
Attachment #8481301 -
Flags: review?(andreea.matei)
Assignee | ||
Comment 28•9 years ago
|
||
All tests are good on ESR24 as well.
Comment 29•9 years ago
|
||
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+
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•