848649 - Update all LOCAL* constants to BASE_URL, TEST_DATA, for consistency and future alternate data locations

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, enhancement)

Description

[Jonathan French (:jfrench)]

Begat from bug 685709, and discussed in cleanup in bug 848361. From my understanding it appears we want to create a uniform constant in all tests that use local input data, to allow mozmill to be capable of optionally accessing it as _remote test input data.

A simple example of the kind of change I believe we are intending to make
testSearch/testGetMoreSearchEngines

const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
const SEARCH_ENGINE_URL = LOCAL_TEST_FOLDER + "search/mozsearch.html";

To be...

const BASE_URL* = collector.addHttpResource("../../../data/");
const SEARCH_ENGINE_URL = BASE_URL + "/search/mozsearch.html";

*or whatever you would like the constant called. BASE_FOLDER, BASE_PATH, etc.


Note there are also many different remote urls accessed in the tests which we expect would remain untouched with this work. They are enumerated here:

http://www.mozqa.com
http://www.mozilla.org
https://ssl-dv.mozqa.com
http://domain1.mozqa.com
https://ssl-selfsigned.mozqa.com
https://mur.at
https://addons.mozilla.org
https://mail.mozilla.org

If I've misunderstood anything, feel free to clarify and comment. Feel free to assign me to do the work, if you'd like.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you for filing this bug. In general everything looks fine. But something we should try out first is if addHttpResource can handle absolute paths correctly. Would you mind to try this out? If we pass in the base url via the automation scripts it will always be an absolute path. Once we proved that I can give you more feedback how it should look like.

Comment 2

[Jonathan French (:jfrench)]

Sure, I will try that out with an existing test and report back.

Comment 3

[Jonathan French (:jfrench)]

Attachment: addHttpRes absolute path fail

Indeed, addHttpResource doesn't appear to like being passed an absolute path, or at least as a url.

(chunk of code tested)
const REMOTE_TEST_FOLDER = collector.addHttpResource("https://www.mozilla.org");
const REMOTE_TEST_PAGE = REMOTE_TEST_FOLDER + "/en-US/about/legal.html";

(result)
$ mozmill --show-all -t tests/functional/testNew/testNewTest.js -b "C:\Program
Files\Mozilla Firefox\firefox.exe"
DEBUG | register | true
DEBUG | mozmill.startRunner | true
ERROR | Test Failure: {"exception": {}}

I've attached the entire test result for reference.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No, you cannot use an URL for that. addHttpResource requires a local path to set the root for the local HTTP server. So please use something like "c:\mozmill-tests\data\" for it.

Comment 5

[Jonathan French (:jfrench)]

Ah, local path for http, I see. I tried "c:\(path)\data" and addHttpResource doesn't appear to like that either. I also tried protecting the backslashes, and a bash-like "/c/(path)/data", but all appear to similarly fail.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Does it work when you put file:// as prefix to the absolute path? If not we should check the code of httpd.js how it works. You can find it here:

http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js

Comment 7

[Jonathan French (:jfrench)]

No, unfortunately file:// as prefix fails also. These were the structures I tried with testClearFormHistory, a known working test:

file:///C:/tmp/ (its appearance in the browser url when viewing the file)
file://C:/tmp/
file://tmp/

const LOCAL_TEST_FOLDER = collector.addHttpResource("file:///C:/tmp/mozmill-tests/default/data");
const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + "/form_manager/form.html";

I will have a look at httpd.js to see how it works and if there is anything obvious I can see.

Comment 8

[Jonathan French (:jfrench)]

I think beginning at line 2430 in httpd.js is where it's testing for what it considers a valid path. I'm going to try some more path structures, but the functions which may be involved in httpd.js, are:

line 2432
registerPathHandler: function(path, handler) {
    // XXX true path validation!
    if (path.charAt(0) != "/")
      throw Cr.NS_ERROR_INVALID_ARG;

line 2444
  registerPrefixHandler: function(path, handler) {
    // XXX true path validation!
    if (path.charAt(0) != "/" || path.charAt(path.length - 1) != "/")
      throw Cr.NS_ERROR_INVALID_ARG;

line 2456
  registerDirectory: function(path, directory) {
    // strip off leading and trailing '/' so that we can use lastIndexOf when
    // determining exactly how a path maps onto a mapped directory --
    // conditional is required here to deal with "/".substring(1, 0) being
    // converted to "/".substring(0, 1) per the JS specification
    var key = path.length == 1 ? "" : path.substring(1, path.length - 1);

    // the path-to-directory mapping code requires that the first character not
    // be "/", or it will go into an infinite loop
    if (key.charAt(0) == "/")
      throw Cr.NS_ERROR_INVALID_ARG;

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you for testing this Jonathan! So it looks like we will have some troubles then, and can't do this change as long as we use httpd.js on a per test basis. I don't think we should wait for our update in the tests and might want to continue in updating all of our constants.

So something which we can do in the interim is the change to BASE_URL in the tests something like:

BASE_URL = %server_url%/data
TEST_URL = BASE_URL + "%path%";

For local test it would be:

BASE_URL = collector.addHttpResource(%path_to_data%);

For multiple tests it would be:

TEST_URLS = [
  BASE_URL + "%path%",
  ...
];

Comment 10

[Jonathan French (:jfrench)]

Got it, I will make the changes described, where applicable. 

I will search default branch first to find all occurrences of collector.addHttpResource(), and generate the list of files involved, to start.

I may wait to begin these changes, until after patch for bug 848653 has been landed in default to sync to that work, since that patch is modifying constants which I think may overlap with these changes. If it isn't landed before, I will continue on, and re-pull default later and push these bug changes, to resolve anything.

In cases liks testSearch/testGetMoreSearchEngines, where the local test page has its constant named something else like SEARCH_ENGINE_URL, I intend to change those to TEST_URL, unless I hear a request otherwise.

And to add clarity here also, the new constant TEST_URL, will be different from TEST_PAGE - which is being used to describe complete external urls in tests like tests/functional/testSecurity/testGreenLarry. We could even improve those later in another bug to make a bigger differentiation so people don't mix and match TEST_PAGE vs. TEST_URL. Like EXTERNAL_URL, or something.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #10)
> In cases liks testSearch/testGetMoreSearchEngines, where the local test page
> has its constant named something else like SEARCH_ENGINE_URL, I intend to
> change those to TEST_URL, unless I hear a request otherwise.

Sounds fine to me. We should be consistent and there might be also other tests which have similar misnamed constants. 

> And to add clarity here also, the new constant TEST_URL, will be different
> from TEST_PAGE - which is being used to describe complete external urls in
> tests like tests/functional/testSecurity/testGreenLarry. We could even
> improve those later in another bug to make a bigger differentiation so
> people don't mix and match TEST_PAGE vs. TEST_URL. Like EXTERNAL_URL, or
> something.

A perfect move. Probably we should really move the tests which are based on remote content to the remote testrun first. As far as I know there is no bug for it yet. Do you want to file it and mark it as blocking this bug?

Comment 12

[Jonathan French (:jfrench)]

So it looks like it will be 76 files to update in the default branch. Cool.

As per Henrik's request, I will file a bug on moving files related to remote (external url) content to the remote testrun. That is a much smaller number of files, and its not related to files involved in the work in this bug. Henrik has indicated it may need to be marked as a blocker for this bug, but I am pretty sure the files that will be patched with this bug will not involve those files. I confess I don't know what the remote testrun framework is yet :)

Also adjusting the title of the bug as per Henrik's suggestion, since we aren't anticipating switching addHttpResource() with this bug, due to the limitations with httpd.js expecting either a leading or a trailing / in all paths, at this time.

Comment 13

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #12)
> Also adjusting the title of the bug as per Henrik's suggestion,

Oh, and please feel free to tune or shorten the bug title as needed. I was still trying to capture the later-use of this work, but it ended up still being a fairly long summary.

Comment 14

[Jonathan French (:jfrench)]

Added bug 849962 for moving remote tests to the remote testrun as a 'see-also' bug. Again, I'm not entirely sure if it is a blocker for this bug or its patch. I defer to Henrik.

Comment 15

[Jonathan French (:jfrench)]

I am starting on this as my top priority bug at present. After this one, I will look at bug 849986, for streamlining the constants in external url tests.

Comment 16

[Jonathan French (:jfrench)]

Henrik, when you say converting to BASE_URL in Comment #9

BASE_URL = %server_url%/data

Are you referring to any tests which reference external domains, like testSecurity/testMixedContentPage below?
const TEST_PAGE = "https://mozqa.com/data/firefox/security/mixedcontent.html";

I'm also including your comments from bug 849986 below, here for reference for myself:
> So in general we have two types of tests:
> 
> 1. A test which has to use a remote page for testing and can't be implemented
> to be local (at least for now).
>
> 2. A test which uses a local test but can be turned via the upcoming BASE_URL
> to use a remote site.
>
> I think that for tests falling under 1) we should not use the BASE_URL constant
> and directly setup the TEST_URL with the full URI. And test cases which can
> exist on both sides have to get the BASE_URL.

Right now, I'm working on converting all the local tests that use collector.addHttpResource() which in theory can be turned later to use a remote site. That's all going well. I'm wondering, based on the above comments, which route to take for tests with tests with external urls. I'm thinking we don't touch them, but feel free to confirm.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #16)
> Henrik, when you say converting to BASE_URL in Comment #9
> 
> BASE_URL = %server_url%/data
> 
> Are you referring to any tests which reference external domains, like
> testSecurity/testMixedContentPage below?
> const TEST_PAGE =
> "https://mozqa.com/data/firefox/security/mixedcontent.html";

No, because for such tests you need a special web server. Especially for testing certificates which are bound to a specific domain. We can't easily switch web servers here.

> remote site. That's all going well. I'm wondering, based on the above
> comments, which route to take for tests with tests with external urls. I'm
> thinking we don't touch them, but feel free to confirm.

Tests which those lines will finally end up in the remote testrun. The constant would have to be renamed to TEST_URL so we stay in sync and as given by the summary.

Comment 18

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Jonathan French (:jfrench) from comment #16)
> 
> > remote site. That's all going well. I'm wondering, based on the above
> > comments, which route to take for tests with tests with external urls. I'm
> > thinking we don't touch them, but feel free to confirm.
> 
> Tests which those lines will finally end up in the remote testrun. The
> constant would have to be renamed to TEST_URL so we stay in sync and as
> given by the summary.

Ok, I will update any tests which use external urls, update their constant to TEST_URL, and include them with this patch. I'm making good progress on the collector.addHttpResource() ones, more than 1/2way done now.

Comment 19

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) - wip

This is a temporary, work-in-progress(wip) patch, do not review.

I've completed a first pass. 76 files modified. This patch represents a resolve against today's latest default branch. 

Templates are still tbd, external url tests are still tbd, I want to re-check some of my edits, plus final testing of all affected tests tbd. Once I've done those four things I will produce a real first patch for review.

This is mostly temporarily storing the work offsite with access to the team (non local to my machine) since it represents a couple days' work so far, including resolves. Just in case I get hit by a bus :)

This patch will be marked as obsolete when a rev1 patch is produced.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 724598 [details] [diff] [review]
const baseurl,testurl update (default) - wip

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

::: lib/tests/testAddonsBlocklist.js
@@ +10,5 @@
>  var modalDialog = require("../modal-dialog");
>  var prefs = require("../prefs");
>  var tabs = require("../tabs");
>  
> +const BASE_URL = collector.addHttpResource("../../data/addons/");

Please let me comment once, so we can fix that asap. The root should not be data/addons but data only. Please see comment 0.

Comment 21

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Comment on attachment 724598 [details] [diff] [review]
> const baseurl,testurl update (default) - wip
> 
> Review of attachment 724598 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/tests/testAddonsBlocklist.js
> @@ +10,5 @@
> >  var modalDialog = require("../modal-dialog");
> >  var prefs = require("../prefs");
> >  var tabs = require("../tabs");
> >  
> > +const BASE_URL = collector.addHttpResource("../../data/addons/");
> 
> Please let me comment once, so we can fix that asap. The root should not be
> data/addons but data only. Please see comment 0.

Thanks, I will adjust them.

Comment 22

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) - wip - rev1.1

Follow on work-in-progress patch, as per my comments in Comment #18. Wip rev 1.1.

No need for review, but if you want to have a look at a first attempt at updating the templates(and its readme) they are included in this temporary patch. Let me know what changes you'd like and I will adjust them.

Comment 23

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) - wip - rev1.2

Follow-on temporary work-in-progress patch, as per my comments in Comment #18. Wip rev 1.2.

Contains fixes to "../data/[a-zA-Z]" tests outlined in Comment #20. Five files adjusted. This patch is a roll up of Wip 1.1, so this also contains the previous first attempt at /templates updates, and its readme.

Still to be done, adding the external url tests, then testing against Default Nightly.

Comment 24

[Jonathan French (:jfrench)]

Taking a brief detour to finish off changes in default for unrelated bug 848361 and bug 848653, before returning to this one. Should be back on this later today.

Comment 25

[Jonathan French (:jfrench)]

So, I'd like to do the work converting external http and ftp url's to TEST_URL, in bug 849986, rather than trying to wrap that change into this already large patch. Does anyone object?

I am beginning initial testing of the 75+ tests, and will do a re-pull and resolve for this patch, when patches for bug 848361, and patch for bug 848653 are landed in default.

Comment 26

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) - wip - rev1.3

Follow-on temporary work-in-progress patch, wip rev 1.3.

Contains a couple of tweaks. Two files adjusted. This patch is a roll up of Wip 1.2, contains tbd reviewed /templates updates, and its readme.

All 76 tests, tested against Default Nightly 20130314030914. All tests pass where expected. 

74 passes, 2 known-failures 
functional/testAddons/testPluginDisableAffectsAboutPlugins
lib/tests/testAddonsBlocklist

Still to be done, a re-pull of default, another push and resolve, testing again against Default Nightly, and a re-check of any whitespace issues.

Comment 27

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #25)
> I am beginning initial testing of the 75+ tests, and will do a re-pull and
> resolve for this patch, when patches for bug 848361, and patch for bug
> 848653 are landed in default.

We landed bug 848653 on default, but I am working on its backports at the moment. I will return to this bug after that. Bug 848361 is yet to be landed on default, but is ready. After landing, that will result in additional resolves for this bug also.

Comment 28

[Jonathan French (:jfrench)]

Bug 848653 is now fixed, bug 848361 yet to be landed; so I am returning to this bug to do an initial pass of resolves on the ~75 files.

Comment 29

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) - wip - rev1.4

Follow-on temporary work-in-progress patch, wip rev 1.4. Eighty files modified.

More resolves against the latest state of Default branch. This patch is a roll up of Wip 1.3, contains /templates updates, and its readme, to be reviewed.

Tested with Default Nightly 22.0a1 20130320031103. All tests pass where expected. 

75 passes, 1 known partial-failure
functional/testAddons/testPluginDisableAffectsAboutPlugins

Still to be done, a re-check of any whitespace issues, plus any requested adjustments to /templates or its readme.

Comment 30

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #29)
> 
> Still to be done, a re-check of any whitespace issues, plus any requested
> adjustments to /templates or its readme.

One week on, I suspect I will need to do more resolves also. I may do some more of those now, while we are finishing up landing bug 848361.

Comment 31

[Jonathan French (:jfrench)]

So far no resolves required, wip rev1.4 can still be applied cleanly on default (not that we are about to do that just yet). Tests continue to pass in default nightly, as per comment #29.

Comment 32

[Jonathan French (:jfrench)]

I have one adjustment to do to testSearch/testOpenSearchAutoDiscovery, at a minimum.

Comment 33

[Jonathan French (:jfrench)]

So, there appear to be several code-styles for existing tests, which use key,value pairs as constants. Let me know which style you want, and I'll do the ones involved in this refactoring, all the same.

(examples of different styles)
testSearch/testOpenSearchAutodiscovery.js
const SEARCH_ENGINE = {
  name: "OpenSearch Test",
  url : LOCAL_TEST_FOLDER + 'search/opensearch.html'
};

testSearch/testAddMozSearchProvider.js
const SEARCH_ENGINE = {name: "mozqa.com",
                       url : LOCAL_TEST_FOLDER + "search/mozsearch.html"};

testAwesomeBar/testCheckItemHighlight.js
const LOCAL_TEST_PAGES = [
  {URL: LOCAL_TEST_FOLDER + 'layout/mozilla_grants.html',
   name: "grants"}
];

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I would personally leave those alone for now and do them in a follow-up bug. For now only observe the last one which should have a name as TEST_URLS.

Comment 35

[Jonathan French (:jfrench)]

Right, in cases where it is a true array of values, like testAwesomeBar/testGoButtons, I have set them to TEST_URLS plural. 

In cases like testCheckItemHighlight above, where it is actually a single key value pair, I actually switched it to singular (ie. I believe testCheckItemHighlight was in error). Since there is only one "thing". There appeared to be precedence for this convention across the tests I looked at, like testSuggestHistory, testSuggestBookmarks, which use singular LOCAL_TEST_PAGE for a single key value pair.

I will do a minor code-block adjustment testCheckItemHighlight though, returning it to its block appearance above, and will freshen up the current patch.

Comment 36

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #35)
> I will do a minor code-block adjustment to testCheckItemHighlight though,
> returning it to its block appearance above, and will freshen up the current
> patch.

Correction, the code block adjustment I'm going to do, is to testOpenSearchAutoDiscovery.

Comment 37

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #35)

I misspoke. It turns out testCheckItemHightlight is the only test with an array which is strangely using a single element like this. testSuggest{History,Bookmarks} were actually correct. Anyway, I could correct testCheckItemHighlight in this patch, or do it in a separate bug, if preferred.

Comment 38

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) - wip - rev1.5

Follow-on temporary work-in-progress patch, wip rev 1.5. Eighty files modified.

Minor code-block revert of testOpenSearchAutoDiscovery.js 

Tested with Default Nightly 23.0a1 20130402030843. All tests pass where expected.

1 known skip
tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart

Still to be done, any requested adjustments to /templates or its readme.

Comment 39

[Jonathan French (:jfrench)]

Comment on attachment 732549 [details] [diff] [review]
const baseurl,testurl update (default) - wip - rev1.5

Now that it looks like we are getting closer to landing similar refactor work for mutt (bug 858731), I'll ask for a partial-review of this current wip 1.5 patch at your convenience, of just the 5 files listed below. I will then adjust these files before an official rev1 patch for full review for default. I am sure the templates will need some changes, as I was only making best-guesses on content improvements.

templates/readme.txt
templates/testEmptyTest.js
templates/testModalDialog.js
templates/testPreferencesDialog.js
templates/testSharedModules.js

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 732549 [details] [diff] [review]
const baseurl,testurl update (default) - wip - rev1.5

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

You mentioned a partly review of the files below. Now I see all of them. :) But beside those nits I will mention inline it looks beautiful! Fantastic work even I can imagine it's a lot of copy and paste. I think that will help us a lot to find consistent patterns for various parts of our tests.

::: templates/testSharedModules.js
@@ +6,2 @@
>  const RELATIVE_ROOT   = '../../lib';
>  const MODULE_REQUIRES = ['UtilsAPI'];

Please file a follow-up bug so we can completely update the templates. This way of loading modules is deprecated for a long time now. :(

@@ +10,1 @@
>  const TIMEOUT = 5000;

I would rename the variable and change the value to something else than 5000 to make it clear.

@@ +10,3 @@
>  const TIMEOUT = 5000;
>  
> +// Include this if opening local or relocatable urls 

Please remove the `local` part here. I think it gives a false assumption. We might add in brackets what relocatable urls mean. Also please remove the trailing white-space.

::: tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +21,2 @@
>    {id: "[e-mail]",
> +   url: BASE_URL + TEST_URL + "extensions/icons.xpi"}

Move BASE_URL up into the const definition for TEST_URL. There is no need that we have to do the concatenation twice here.

::: tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension/test1.js
@@ +9,5 @@
>  var tabs = require("../../../../lib/tabs");
>  
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_URL = BASE_URL + "addons/blocklist/" +
> +                 "hardblock_extension/blocklist.xml";

A single string in the second line also exceed the 80 char limit?

::: tests/functional/testAwesomeBar/testPasteLocationBar.js
@@ +10,5 @@
>  
>  const TIMEOUT = 5000;
>  
> +const BASE_URL = collector.addHttpResource('../../../data/');
> +const TEST_URL = BASE_URL + "awesomebar/copypaste.html"; 

nit: trailing whitespace.

::: tests/functional/testSearch/testAddMozSearchProvider.js
@@ +11,5 @@
>  const TIMEOUT_INSTALL_DIALOG = 30000;
>  
> +const BASE_URL = collector.addHttpResource('../../../data/');
> +const TEST_URL = {name: "mozqa.com",
> +                  url : BASE_URL + "search/mozsearch.html"};

Please keep those two lines before any other const declaration in our tests. I think that would be better for consistency.

::: tests/functional/testSearch/testOpenSearchAutodiscovery.js
@@ +8,5 @@
>  
>  const TIMEOUT_INSTALLATION = 30000;
>  
> +const BASE_URL = collector.addHttpResource('../../../data/');
> +const TEST_URL = {

I wouldn't rename this given that it is not an URL only. For constructs like those I would keep the name. This would also apply to the other instances of similar hashes.

::: tests/functional/testSecurity/testGreyLarry.js
@@ +6,5 @@
>  var { assert, expect } = require("../../../lib/assertions");
>  var utils = require("../../../lib/utils");
>  
> +const BASE_URL = collector.addHttpResource("../../../data/");
> +const TEST_URL = BASE_URL + 'layout/mozilla.html';

Can you file a follow-up bug re consistency for double and single quotes? We shouldn't mix them.

Comment 41

[Jonathan French (:jfrench)]

Great thanks Henrik! I will make all these changes and file new bugs where applicable, and will upload a true "rev1" patch for this bug for review, after that.

Comment 42

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #40)
 
> Please file a follow-up bug so we can completely update the templates. This
> way of loading modules is deprecated for a long time now. :(

Now entered as bug 861216.

> Can you file a follow-up bug re consistency for double and single quotes? We
> shouldn't mix them.

Now entered as bug 861209.

Comment 43

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #40)
> Review of attachment 732549 [details] [diff] [review]:

Ok, all the changes described in Comment#40 have been made locally, and I still need to run some tests with Default Nightly first, before uploading a rev1 patch. But to touch on some of the review comments prior to that:

> :::
> tests/functional/restartTests/
> testAddons_installUninstallHardBlocklistedExtension/test1.js
> > +const TEST_URL = BASE_URL + "addons/blocklist/" +
> > +                 "hardblock_extension/blocklist.xml";
> 
> A single string in the second line also exceed the 80 char limit?

Yes, unfortunately so. 81 characters, I had elected to leave it for that reason. If you'd like me to consolidate it to one line at 81 I can.
 

> ::: tests/functional/testSearch/testAddMozSearchProvider.js
> @@ +11,5 @@
> Please keep those two lines before any other const declaration in our tests.
> I think that would be better for consistency.

I've moved BASE_URL,TEST_URL consts directly under the Module vars, in cases where that needed to occur. In some cases, TIMEOUT consts with default values of 5000 have been left above, because I have an upcoming patch which will purge them anyway, via bug 848200.

I will upload a new patch after testing, for official rev1 review hopefully sometime tomorrow.

Comment 44

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) rev1.0

Patch "const baseurl,testurl update (default) rev1.0" for the Default branch. Eighty files modified.

All code changes have been made according to the comments above, and are complete. The templates definitely look improved with the const reordering and comment changes. They will receive further work in bug 861216.

Patch has been reapplied to the latest Default pull, for landing. It didn't require any resolves at this time.

2 known skips
restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test1.js
testAwesomeBar/testLocationBarSearches.js (known skip, also passed fine earlier)

Tested with Default Nightly 23.0a1 20130413030927. All other tests pass as expected.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 737165 [details] [diff] [review]
const baseurl,testurl update (default) rev1.0

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

That looks fantastic. Just some more nits and the before mentioned change on the mutt tests we would have to do here too. We are close to its landing! \o/

::: lib/tests/testAddonsBlocklist.js
@@ +12,5 @@
>  var tabs = require("../tabs");
>  
> +const BASE_URL = collector.addHttpResource("../../data/");
> +const TEST_URL = BASE_URL +
> +                 "addons/blocklist/mixedblock_extension/blocklist.xml";

Similar to the Mutt tests for Mozmill itself, the proposal from Dave sounds ideal and we should also update the constant name for the Firefox tests and make use of TEST_LOCATION(S).

::: templates/testEmptyTest.js
@@ +2,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/. */
>  
> +// Include required modules
> +const RELATIVE_ROOT = '../../lib';

Why has this been added in this patch? We don't use it anymore, so it can be removed again.

::: templates/testPreferencesDialog.js
@@ +12,5 @@
> +// Include this if opening external urls
> +const TEST_URL = "http://www.external.url/path/file.html";
> +
> +// Include this only if specifying a timeout other than 5000ms
> +const TEST_EXAMPLE_TIMEOUT = 3000;

For now I would only include this into the empty template. The other templates don't really make use of external test data. We can improve all the content later in the follow-up bug you have already filed.

::: tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js
@@ +8,5 @@
>  var prefs = require("../../../../lib/prefs");
>  var tabs = require("../../../../lib/tabs");
>  
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_URL = BASE_URL + "addons/install.html?addon=";

I would kill TEST_URL here and copy the string into the ADDON declaration. I know it will exist twice then but it's not really a test.

::: tests/functional/restartTests/testAddons_installTheme/test1.js
@@ +8,5 @@
>  var prefs = require("../../../../lib/prefs");
>  var tabs = require("../../../../lib/tabs");
>  
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_URL = BASE_URL + "addons/install.html?addon=";

Same as above.

::: tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension/test1.js
@@ +9,5 @@
>  var tabs = require("../../../../lib/tabs");
>  
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_URL = BASE_URL + "addons/blocklist/" +
> +                 "hardblock_extension/blocklist.xml";

Will we exceed the 80 char limit when the whole string is put in the second line?

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #43)
> > testAddons_installUninstallHardBlocklistedExtension/test1.js
> > > +const TEST_URL = BASE_URL + "addons/blocklist/" +
> > > +                 "hardblock_extension/blocklist.xml";
> > 
> > A single string in the second line also exceed the 80 char limit?
> 
> Yes, unfortunately so. 81 characters, I had elected to leave it for that
> reason. If you'd like me to consolidate it to one line at 81 I can.

We don't have such a hard limitation. Given the improved readability it would make sense to put the whole string in the second line.

Comment 47

[Jonathan French (:jfrench)]

I will make those changes in that case, and the others as described.

Comment 48

[Jonathan French (:jfrench)]

I have made all the changes locally, except the first one. 

> +const TEST_URL = BASE_URL +
> +                 "addons/blocklist/mixedblock_extension/blocklist.xml";
> Similar to the Mutt tests for Mozmill itself, the proposal from Dave sounds
> ideal and we should also update the constant name for the Firefox tests and
> make use of TEST_LOCATION(S).

To clarify before I begin this part, can you describe a criteria I can use to determine what constants and files should be modified here. I'd like to make the changes successfully on first patch attempt :). This test above for example and the code excerpted, doesn't have a plurality of url's. The test does have several addons. Dave's example in the Mutt tests, had needed TEST_URL,TEST_URLS in the same file, which was a different scenario again, although that part I understood for a conversion to TEST_LOCATIONS.

If it's a short list, and you want to just provide the list, that's fine also.

Comment 49

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) rev1.1 - wip

Patch "const baseurl,testurl update (default) rev1.1 - wip" work-in-progress patch for temporary storage purposes only. Seventy nine files modified.

All code changes have been made according to the review comments, except for the outstanding TEST_LOCATION(S) change referenced in Comment#48; pending clarification how I can isolate the tests which require that change.

This wip patch has also been sync'd to the latest Default pull, which did require several resolves. 

One of which was the removal of testAwesomeBar/testLocationBarSearches, that test has been retired from Default (bug 860330).

Comment 50

[Jonathan French (:jfrench)]

So we just landed bug 848200 and as expected, that resulted in quite a few resolves required here. I will do those and upload a new work in progress patch after that. 

If if there's guidance on my Comment#48, for the selection criteria to determine where the additional TEST_LOCATION(S) gets implemented, that will be helpful also.

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #48) 
> To clarify before I begin this part, can you describe a criteria I can use
> to determine what constants and files should be modified here. I'd like to
> make the changes successfully on first patch attempt :). This test above for
> example and the code excerpted, doesn't have a plurality of url's. The test
> does have several addons. Dave's example in the Mutt tests, had needed
> TEST_URL,TEST_URLS in the same file, which was a different scenario again,
> although that part I understood for a conversion to TEST_LOCATIONS.

It's probably hard to get them all right without checking the tests. Maybe we might want to iterate and clarify on IRC. But for now I think:

* BASE_URL - The base URL for test pages which can be local or remote
* TEST_LOCATION(S) - URL(S) for test pages used in the test
* ADDON(S) - Details for add-ons used in the test
* SEARCH_ENGINE(S) - Details for search engines used in the test

Most likely there are others which should get names accordingly to their type of data.

Comment 52

[Jonathan French (:jfrench)]

Sounds fine, I understand now, but sure we can iterate on IRC if needed. I will pose one idea to consider, which is to abbreviate TEST_LOCATION(S) to TEST_LOC(S), as this will make for a nice alignment of code blocks and also prevent further line wraps with lengthy locations. Where TEST_LOCATION(S), will push numerous paths beyond 80 characters.

eg.
const BASE_URL = collector.addHttpResource("../../../../data/");
const TEST_LOC = BASE_URL + "addons/blocklist/hardblock_extension/blocklist.xml";

What do you guys think? I will proceed with resolves first, about two dozen of them, so that will occupy some time before I/we start this change above.

Comment 53

[Jonathan French (:jfrench)]

Some other groups of ideas, while I'm mid-resolve, for TEST_LOCATION(S):

FILE_URL
FILE_LOC
FILE_SRC

or

PAGE_URL
PAGE_LOC
PAGE_SRC

or

DATA_URL
DATA_LOC
DATA_SRC

Since technically the 'thing' being opened, isn't a test itself. The test is the thing being run. 

I kind of liked DATA_SRC, since it is universal. Could be a .xul, could be a .htm, could represent anything. I'm easy though, just throwing out ideas.

Comment 54

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #53)
> Some other groups of ideas, while I'm mid-resolve, for TEST_LOCATION(S):

And I suppose TEST_SRC could be an option also.

Comment 55

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) rev1.6 - wip

Follow-on work-in-progress patch, wip rev 1.6. Seventy nine files modified.

This latest wip patch represents about 30 resolves to sync with the latest Default.

Tested with Default Nightly 23.0a1 20130424030917. All tests pass where expected.

1 known skip
tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart

1 test was removed from repo by other work
tests/functional/testAwesomeBar/testLocationBarSearches

Still to be done, changing all TEST_URL's to something; as per Dave and Henrik's call, in Comment#51 through Comment#53.

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #53)
> I kind of liked DATA_SRC, since it is universal. Could be a .xul, could be a
> .htm, could represent anything. I'm easy though, just throwing out ideas.

We not only have information about references. What if we simply use TEST_DATA whether if it is singular or plural? The thing with TEST_LOC(ATION) is that people still might be confused because the test is the actual Mozmill file. We should make it clear that we are referencing a testcase file or additional data here.

Dave, what do you think?

Comment 57

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I like TEST_DATA

Comment 58

[Jonathan French (:jfrench)]

Ok, I will begin this change. All TEST_URL,TEST_URLS will be converted to TEST_DATA, along with other adjustments for ADDON(S), or SEARCH_ENGINE(S) where applicable.

For related bug 858731, will you want to resolve the need for both forms in a single test(TEST_URL,TEST_URLS) to be (TEST_DATUM,TEST_DATA)? Where TEST_DATUM would be the rare case to facilitate it. This is the only test I am aware of so far across the repos, but we should probably agree on in advance how we will handle it in case other cases crop up later.

(mutt/mutt/tests/js/utils/pageload.js)
https://bugzilla.mozilla.org/attachment.cgi?id=735751&action=diff#a/mutt/mutt/tests/js/utils/pageload.js_sec1

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #58)
> For related bug 858731, will you want to resolve the need for both forms in
> a single test(TEST_URL,TEST_URLS) to be (TEST_DATUM,TEST_DATA)? Where
> TEST_DATUM would be the rare case to facilitate it. This is the only test I
> am aware of so far across the repos, but we should probably agree on in
> advance how we will handle it in case other cases crop up later.

I don't think we should ever use TEST_DATUM which also sounds awkward. But not sure if I misunderstood your question here.

Comment 60

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I agree, I don't like TEST_DATUM. I don't have a good suggestion for this test though.. My only thought is that we could modify the array to look something like:

const TEST_DATA = {
  container: "BASE_URL +  "iframe.html",
  locations: [
    {url: BASE_URL + "form.html", type: "ID", value: "fname"},
    {url: BASE_URL + "link.html", type: "ID", value: "link"},
    {url: BASE_URL + "singlediv.html", type: "ID", value: "test-div"}
  ]
}

Comment 61

[Jonathan French (:jfrench)]

Cool, that's fine. I don't like DATUM either, I just wanted to note that test which will require some alternate solution. I will modify to Dave's structure when I get back to that bug.

I'm nearly done re-checking TEST_URL(S) to TEST_DATA change across the 70+ files for this bug, I just need to do a new pull and resolve, and another test run to confirm the test results are consistent with earlier patches. I will provide a new patch after that.

While checking for code block alignment, I also updated any affected lines, converting single-quoted path strings to double quotes.

Comment 62

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) rev1.1

Patch "const baseurl,testurl update (default) rev1.1" for the Default branch. Eighty four files modified.

This patch encompasses the work to:
1) reconvert all const TEST_URL(S) to TEST_DATA
2) adjust code block alignment where affected by above
3) recheck/adjust all BASE_URL,TEST_DATA to live directly under requires block
4) convert any lines involved, from single quoted strings to double quoted
5) recheck all ADDON(S) and SEARCH_ENGINE(S) are meeting spec 

Patch has been resolved to the latest Default pull.

5 known skips
functional/restartTests/testAddons_changeTheme/test1
functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test1
functional/testAddons/testPluginDisableAffectsAboutPlugins
functional/testSecurity/testSSLDisabledErrorPage
functional/testSecurity/testUnknownIssuer

There may be more work to be done, but setting Dave and Henrik for initial review.

Tested with Default Nightly 23.0a1 20130429030926. All other tests pass as expected.

Comment 63

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 743211 [details] [diff] [review]
const baseurl,testurl update (default) rev1.1

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

Wow, a massive patch! This looks fantastic, I can't wait to see it landed. You have my r+ with the nit addressed, but you mentioned there may be more work to be done? Also, could you send some report results to mozmill-crowd to demonstrate there are no regressions from these changes. Append --report=http://mozmill-crowd.blargon7.com/db/ to your mozmill-automation command line to submit the reports.

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +8,5 @@
>  var prefs = require("../../../lib/prefs");
>  var toolbars = require("../../../lib/toolbars");
>  
> +const BASE_URL = collector.addHttpResource("../../../data/");
> +const TEST_DATA = [

Nit: With only one item, this doesn't need to be an array.

Comment 64

[Jonathan French (:jfrench)]

Actually there was no intended work to be done, I was just anticipating there may be some tweaks. I will adjust testCheckItemHighlight accordingly. 

I will send a full test result to mozmill-crowd also. I have been diffing my local run results all the way along with each patch and build, so I was able to visually see the deltas in the test runs so far, and everything has looked in order.

I will provide an updated patch with that tweak shortly.

Comment 65

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (default) rev1.2

Patch "const baseurl,testurl update (default) rev1.2" for the Default branch. Eighty four files modified.

Just a minor update to previous rev1.1 patch removing unused array from functional/testAwesomeBar/testCheckItemHighlight, which passes fine after the change. There are no other changes.

Tested with Default Nightly 23.0a1 20130430030941. Will submit a run to mozmill-crowd shortly.

Comment 66

[Jonathan French (:jfrench)]

(In reply to Dave Hunt (:davehunt) from comment #63)
> Comment on attachment 743211 [details] [diff] [review]
> Also, could you send some report results to mozmill-crowd
> to demonstrate there are no regressions from these changes.

Had to spend some time sorting out another MinGW error with mozmill-automation (pip install mercurial was failing), I had to:

o add C:\MinGW\bin to my system path
o add C:\mozilla-build15\python\Lib\distutils.cfg 
[build]
compiler=mingw32

Once I did that, I was able to `pip install mercurial`, and produce 5 of the available 7 test runs. Functional, endurance, update, remote, l10n.

http://mozmill-crowd.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa0615cd52
http://mozmill-crowd.blargon7.com/#/endurance/report/452ec32f8deec0960aea87aa0615d583
http://mozmill-crowd.blargon7.com/#/update/report/452ec32f8deec0960aea87aa0615e55a
http://mozmill-crowd.blargon7.com/#/remote/report/452ec32f8deec0960aea87aa0615f32a
http://mozmill-crowd.blargon7.com/#/l10n/report/452ec32f8deec0960aea87aa0615f3d4

Addons and compat_addons fail to run, for reasons unrelated to my patch. I am including the errors here for posterity. Running a fresh mozmill-automation distribution with mozmill 1.5.21, WindowsXP SP3.

(testrun_addons.py)
Traceback (most recent call last):
  File "./testrun_addons.py", line 51, in <module>
    main()
  File "./testrun_addons.py", line 46, in main
    AddonsTestRun().run()
  File "c:\tmp\mozmill-automation\libs\testrun.py", line 439, in run
    raise self.last_exception
AttributeError: 'AddonsTestRun' object has no attribute 'target_addon'

(testrun_compat_addons.py) 
Traceback (most recent call last):
  File "./testrun_compat_addons.py", line 10, in <module>
    from libs.compat_by_default import CompatibleByDefault
  File "c:\tmp\mozmill-automation\libs\compat_by_default.py", line 12, in <module>
    from mozdownload import ReleaseScraper
ImportError: No module named mozdownload

I am also curious why the test run results show all the Add-ons associated with what I think of as my Release install of Firefox. 

I gather when running on a development system and not a clean VM, they will simply show up in the test run regardless of the build stream, and this is hopefully normal (ie. Default Nightly, Aurora, Beta, Release).

Comment 67

[Jonathan French (:jfrench)]

Attachment: const baseurl,testdata update (default) rev1.2a

Patch "const baseurl,testdata update (default) rev1.2a" for the Default branch.

Identical in all respects to patch rev1.2 reviewed by Dave in Comment#65. Just updated the commit message to reflect the change from TEST_URL to TEST_DATA. Switching reviewer to Henrik as Dave is unavailable for a few days.

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #66)
> Had to spend some time sorting out another MinGW error with
> mozmill-automation (pip install mercurial was failing), I had to:
> 
> o add C:\MinGW\bin to my system path
> o add C:\mozilla-build15\python\Lib\distutils.cfg 
> [build]
> compiler=mingw32
> 
> Once I did that, I was able to `pip install mercurial`, and produce 5 of the
> available 7 test runs. Functional, endurance, update, remote, l10n.

Aren't you using our beta of mozmill-environment? I believe we mentioned that already. It comes with all the necessary tools pre-installed. You can find it here: https://mozqa.com/mozmill-env/ 

> (testrun_addons.py)
> Traceback (most recent call last):
>   File "./testrun_addons.py", line 51, in <module>
>     main()
>   File "./testrun_addons.py", line 46, in main
>     AddonsTestRun().run()
>   File "c:\tmp\mozmill-automation\libs\testrun.py", line 439, in run
>     raise self.last_exception
> AttributeError: 'AddonsTestRun' object has no attribute 'target_addon'

What was the command line you were using here? Might be we have to file as a new bug in Mozilla QA / Mozmill Automation. Were you running with the --with-untrusted option? It might be that this is the cause.

> (testrun_compat_addons.py) 
> Traceback (most recent call last):
>   File "./testrun_compat_addons.py", line 10, in <module>
>     from libs.compat_by_default import CompatibleByDefault
>   File "c:\tmp\mozmill-automation\libs\compat_by_default.py", line 12, in
> <module>
>     from mozdownload import ReleaseScraper
> ImportError: No module named mozdownload

Use the mozmill-env and mozdownload is pre-installed.

> I am also curious why the test run results show all the Add-ons associated
> with what I think of as my Release install of Firefox. 

Those are installed plugins on your system and are used automatically across different profiles.

Comment 69

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #68)
> 
> Aren't you using our beta of mozmill-environment? I believe we mentioned
> that already. It comes with all the necessary tools pre-installed. You can
> find it here: https://mozqa.com/mozmill-env/

I'm using a full mozilla-build 1.5.21 l10n shell environment, not mozmill-env. The version of python in mozilla-build l10n shows as 2.7.2.

Should all 7 run successfully in mozilla-build?

> 
> > (testrun_addons.py)
> > Traceback (most recent call last):
> >   File "./testrun_addons.py", line 51, in <module>
> >     main()
> >   File "./testrun_addons.py", line 46, in main
> >     AddonsTestRun().run()
> >   File "c:\tmp\mozmill-automation\libs\testrun.py", line 439, in run
> >     raise self.last_exception
> > AttributeError: 'AddonsTestRun' object has no attribute 'target_addon'
> 
> What was the command line you were using here? Might be we have to file as a
> new bug in Mozilla QA / Mozmill Automation. Were you running with the
> --with-untrusted option? It might be that this is the cause.

./testrun_addons.py "C:\Program Files\Mozilla Firefox Default\firefox.exe" --repository /c/tmp/blargon/mozmill-tests/ --report=http://mozmill-crowd.blargon7.com/db/


> > (testrun_compat_addons.py) 
> > Traceback (most recent call last):
> >   File "./testrun_compat_addons.py", line 10, in <module>
> >     from libs.compat_by_default import CompatibleByDefault
> >   File "c:\tmp\mozmill-automation\libs\compat_by_default.py", line 12, in
> > <module>
> >     from mozdownload import ReleaseScraper
> > ImportError: No module named mozdownload
> 
> Use the mozmill-env and mozdownload is pre-installed.

I will try mozmill-env again. The last time I tried it was about 6 weeks ago, at that time I encountered a variety of failures(at least on Windows) and Dave at the time said it wasn't ready for prime time yet, and directed me to use mozilla-build l10n for the time being. I can't remember the bug thread offhand, but I can dig it up if needed. I will try latest mozmill-env again though.

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #69)
> > Aren't you using our beta of mozmill-environment? I believe we mentioned
> > that already. It comes with all the necessary tools pre-installed. You can
> > find it here: https://mozqa.com/mozmill-env/
> 
> I'm using a full mozilla-build 1.5.21 l10n shell environment, not
> mozmill-env. The version of python in mozilla-build l10n shows as 2.7.2.
> 
> Should all 7 run successfully in mozilla-build?

I would really advice you to make the flip over to mozmill-env. We don't make use of MozillaBuild for a long time and all of our jobs are getting run through mozmill-env. So I can't say how it behaves.

> > > (testrun_addons.py)
> > > Traceback (most recent call last):
> > >   File "./testrun_addons.py", line 51, in <module>
> > >     main()
> > >   File "./testrun_addons.py", line 46, in main
> > >     AddonsTestRun().run()
> > >   File "c:\tmp\mozmill-automation\libs\testrun.py", line 439, in run
> > >     raise self.last_exception
> > > AttributeError: 'AddonsTestRun' object has no attribute 'target_addon'
> > 
> > What was the command line you were using here? Might be we have to file as a
> > new bug in Mozilla QA / Mozmill Automation. Were you running with the
> > --with-untrusted option? It might be that this is the cause.
> 
> ./testrun_addons.py "C:\Program Files\Mozilla Firefox Default\firefox.exe"
> --repository /c/tmp/blargon/mozmill-tests/
> --report=http://mozmill-crowd.blargon7.com/db/

Please add `--with-untrusted`. I believe the failure will be gone? If that's the case it's worth a new bug. Looks like it's a side effect of bug 853005 where we now re-raise exceptions. This one seems to fall through.

Comment 71

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #70)
> > ./testrun_addons.py "C:\Program Files\Mozilla Firefox Default\firefox.exe"
> > --repository /c/tmp/blargon/mozmill-tests/
> > --report=http://mozmill-crowd.blargon7.com/db/
> 
> Please add `--with-untrusted`. I believe the failure will be gone? If that's
> the case it's worth a new bug. Looks like it's a side effect of bug 853005
> where we now re-raise exceptions. This one seems to fall through.

`--with-untrusted` allows testrun_addons.py to work, at least in a mozilla-build shell, where without it it fails. I will go to a mozmill-env again and try to reproduce the behaviour there.

Comment 72

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jonathan French (:jfrench) from comment #71)
> `--with-untrusted` allows testrun_addons.py to work, at least in a
> mozilla-build shell, where without it it fails. I will go to a mozmill-env
> again and try to reproduce the behaviour there.

So it's a bug we should log separately as mentioned above. Shouldn't be hard to get fixed with a better failure message. Thanks.

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 744218 [details] [diff] [review]
const baseurl,testdata update (default) rev1.2a

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

All fine now! I will get it landed soon.

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+2]]

http://hg.mozilla.org/qa/mozmill-tests/rev/f4370e762026