858731 - Update mutt test constants to BASE_URL,TEST_DATA

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jonathan French (:jfrench)]

Related to the work in bug 848649 for Mozmill to support bug 685709, I noticed that Mutt tests appear to require similar work to be consistent in their use of BASE_URL,TEST_URL. I'm happy to start on this work in parallel if required.

eg.
https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/controller/dnd_content_chrome.js

At first glance, there appear to be about 20 or so files involved. If assigned I will dig around more deeply and try to find them all, there may be more, using different naming conventions.

Comment 1

[Jonathan French (:jfrench)]

Actually, this issue should be raised on Github rather than Bugzilla, correct?

Comment 2

[Jonathan French (:jfrench)]

Ignore the last comment, I was thinking mozmill-environment.

Comment 3

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

Sure. Sounds good.

Comment 4

[Jonathan French (:jfrench)]

Would you like consistency-refactors of chrome XUL url's, to TEST_URL, also?

eg. /mutt/mutt/tests/js/controller/dnd_chrome.js
controller.open("chrome://mozmill/content/test/test.xul");

Comment 5

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

It's a test so yes please.

Comment 6

[Jonathan French (:jfrench)]

Ok, I have a preliminary set of changes I've pushed to my Github fork. Twenty seven files modified in Mutt. However I still need to understand a bunch of Mutt failures first before issuing a pull request.
https://github.com/tojonmz

Once I've cleared them up and understand the failures aren't regressions, I will do that. 

Mutt is quite challenging, because it appears you can't run a single test; at a minimum you have to run a whole sub-directory controlled by a manifest.ini. Which, if an early test in the .ini kills Mutt, you can't get a result for the later tests you edited. I suppose one can temporarily modify the .ini and change the order of the tests so your edited ones are run first. It would be really nice to have a mutt -t. I couldn't find any in the usage manual or elsewhere.

Comment 7

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

None of the tests should *kill* mutt. Not sure what in details you are seeing but it might help to describe it.

Comment 8

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> None of the tests should *kill* mutt. Not sure what in details you are
> seeing but it might help to describe it.

Yup, will do. I will try to narrow down what I was seeing late last night, and will report it as a separate bug if so. 

There was some cases where Mutt was failing with known-failures, then proceeding to the next test in the manifest, which of course was fine. However there were cases where Mutt simply appeared to exit mid-run, failing to create the localhost port. That part I'm trying to get a reliable case which reproduces with a clean master.

Comment 9

[Jonathan French (:jfrench)]

Ok I'm pretty satisfied with the results of the Mutt test run. Twenty seven files modified, tested with Default Nightly, and Aurora(where required).

Tested with Default Nightly 23.0a1 20130409030855.
Tested with Aurora 22.0.a2 20130409004013 (for python/js-tests/expectstack.js)

I have provided comments on the changes in the commit below, in my branch. Once everything is to everyone's satisfaction, I'll submit a pull request after that.

https://github.com/tojonmz/mozmill/commit/ad5d625e372faf3a1e80cf250507be4a1263fd1c

Comment 10

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

Jonathan, please create a diff from your work and attach it as usual patch to this bug:
https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Ready_for_a_review

Comment 11

[Jonathan French (:jfrench)]

Sure, will do. Coming up.

Comment 12

[Jonathan French (:jfrench)]

Attachment: const baseurl,testurl update (master)

Patch "const baseurl,testurl update (master)" for mutt Master. Twenty seven files modified.

Rebased recently to apply cleanly to the latest Mutt master.

Tested with Default Nightly 23.0a1 20130409030855. Tests pass where expected.

Transposing my comments from my GitHub commit above, here for benefit:

All of the 27 Mutt tests above are passing with Default Nightly, along with existing, known-failures. These four tests below will require extra attention in the review. Since they can't be run successfully to confirm the change.

(known-fail) mutt/mutt/tests/js/elementslib/mozelement.js (bug 859844)
(known-fail) mutt/mutt/tests/js/elementslib/stale_element.js (bug 761600)
(known-fail) mutt/mutt/tests/js/elementslib/utf-8.js (bug 761603)
(known-fail) mutt/mutt/tests/js/utils/pageload_bfcache.js (bug 760720)

These two tests below are actually intended failures, so their behavior in continuing to fail, is correct. In their case, addHttpResource was not used, as no file was loaded with the tests. So that line was removed.
(intended-fail) mutt/mutt/tests/python/js-tests/test_module2.js
(intended-fail) mutt/mutt/tests/python/js-tests/test_module3.js

Chrome XUL url's were also refactored, as requested.

Comment 13

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

Comment on attachment 735751 [details] [diff] [review]
const baseurl,testurl update (master)

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

This looks good to me, and I can confirm no additional test failures are caused when applying this patch. Asking for feedback from Henrik on the TEST_LOCATIONS suggestion before merging.

::: mutt/mutt/tests/js/utils/pageload.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +const BASE_URL = collector.addHttpResource('../_files/');
> +const TEST_URL = BASE_URL +  "iframe.html";
> +const TEST_URLS = [

It's a little confusing to have both TEST_URL and TEST_URLS, and these aren't strictly just URLS, so perhaps we should call them TEST_LOCATIONS.

Comment 14

[Jonathan French (:jfrench)]

For what its worth, Henrik's comment in the related refactoring for mozmill pointed to that general convention of _URLS. I agree it's an unusual case, where this test actually has to contain both. It's the first time I encountered that scenario in all the refactoring(about 100 files so far) where you couldn't really integrate it into a single entity. Happy to modify as needed, whatever you guys require.
https://bugzilla.mozilla.org/show_bug.cgi?id=848649#c9

Comment 15

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

Comment on attachment 735751 [details] [diff] [review]
const baseurl,testurl update (master)

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

Looks fine! See comments inline for things we should address and which were still open for discussion since Dave reviewed it end of last week.

::: mutt/mutt/tests/js/l10n/getProperty.js
@@ +3,4 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  const { getProperty } = require("l10n");
> +const TEST_URL = "chrome://global/locale/languageNames.properties";

nit: please keep an empty line between the requires and the constant declarations.

::: mutt/mutt/tests/js/utils/pageload.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +const BASE_URL = collector.addHttpResource('../_files/');
> +const TEST_URL = BASE_URL +  "iframe.html";
> +const TEST_URLS = [

Sounds like a better idea. I would support that proposal. It binds it lesser to URLs and let us use even objects. Jonathan, feel free to do a global change and rename it. We should do the same for our mozmill-tests for Firefox too.

Comment 16

[Jonathan French (:jfrench)]

Sounds good. I'm mid-way through review changes for the same mozmill-tests refactor (bug 848649), so will complete that one first, and return to this after that.

Comment 17

[Jonathan French (:jfrench)]

We may use a value like

FILE_SRC
FILE_LOC
DATA_SRC
...

tbd, pending the work in bug 848649, and its resolution there.

Comment 18

[Jonathan French (:jfrench)]

We went with BASE_URL,TEST_DATA for relocatable tests, and TEST_DATA for any tests with external, presently non-relocatable urls, as per bug 848649 and bug 849986. 

I will begin converting the tests in this bug also, and re-testing.

Comment 19

[Jonathan French (:jfrench)]

Attachment: const baseurl,testdata update (master) rev1.1

Patch "baseurl,testdata update (master) rev1.1" for Master. Seventeen files modified.

This work represents the conversion to BASE_URL,TEST_DATA, for all relocatable urls in mutt.

Extensive testing was performed comparing clean Master vs. my branch, to exclude differences in run-to-run results (unrelated to these changes) which was the cause of the extra evaluation time. Up to a dozen test runs of some manifests, in some cases. Patch has also been rebased against latest Master.

(notable file for review)
mutt/mutt/tests/js/utils/pageload.js

This test above, had some more significant changes, to allow the use of a single TEST_DATA constant. It adds element 7 to the array, which gets excluded in the initial for loop, and then used later in the test. It seems to work fine.

(disabled tests)
mutt/mutt/tests/js/elementslib/stale_element.js
mutt/mutt/tests/js/elementslib/utf-8.js

These two tests above, are presently disabled. So they just need careful review, also since utf-8 can't be run to confirm the changes.

Tested with Default Nightly 23.0a1 20130508031113. All tests pass where expected, and produce the same result as current Master.

Comment 20

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

Comment on attachment 747541 [details] [diff] [review]
const baseurl,testdata update (master) rev1.1

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

First, I love this patch! Similar to the one for our tests it makes tests way better understandable. Thanks Jonathan!

Beside that I have some comments which would need to be addressed. See inline.

::: mutt/mutt/tests/js/l10n/getEntity.js
@@ +3,4 @@
>   * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  const { getEntity } = require("l10n");
> +const TEST_DATA = "chrome://branding/locale/brand.dtd";

nit: Please add a blank line in-between.

::: mutt/mutt/tests/js/liveweb_tests/flash-protectedmode.js
@@ +2,4 @@
>   * 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/. */
>  
> +const BASE_URL = "http://www.mozqa.com/";

Given that this will be always remote you can remove that constant.

::: mutt/mutt/tests/js/utils/pageload.js
@@ +31,5 @@
>    /**
>     * PART I - Check different types of pages
>     */
> +  // We omit Container page, array element 7 in these loads
> +  for (var i = 0; i < 7; i++) {

Could we do something like `i in [0, 1, 2, 3, 4, 5, 6]`? That way we could directly specify the items from the list we want to check.

::: mutt/mutt/tests/js/utils/pageload_bfcache.js
@@ +1,3 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */

Was this change intended?

::: mutt/mutt/tests/js/utils/savescreenshot.js
@@ +4,4 @@
>  
>  var utils = {}; Cu.import('resource://mozmill/stdlib/utils.js', utils);
>  
> +const NAME = "smile5x5";

I would declare this below the TEST_DATA constant. On the other side I wonder if we should keep it in the test itself.

Comment 21

[Jonathan French (:jfrench)]

Cool, thanks Henrik. I will provide a revised patch. Thanks for catching that header diff, I had a funky merge to deal with a couple ago, that was one of the by-products of it.

Comment 22

[Jonathan French (:jfrench)]

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

Patch "baseurl,testdata update (master) rev1.2" for Master. Twenty-seven files modified. For some reason in Comment#19 I stated there were seventeen; there are twenty-seven.

This work comprises tweaks from code review Comment#20.

The overall work represents updates to BASE_URL,TEST_DATA; making consistent relocatable and external urls in mutt.

(mutt/mutt/tests/js/utils/pageload.js)
Additionally created a var n in the modified for loop requested by Henrik - to avoid potential conflict with an existing var i declared later in the same function.

Tested with Default Nightly 23.0a1 20130513031057. Tweaked tests pass where expected, and produce the same result as current Master.

Comment 23

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

Comment on attachment 749007 [details] [diff] [review]
const baseurl,testdata update (master) rev1.2

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

With the issue mentioned inline fixed, we should be good to get this landed.

::: mutt/mutt/tests/js/liveweb_tests/flash-protectedmode.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/. */
>  
> +const TEST_DATA  "http://www.mozqa.com/data/firefox/plugins/flash/" +
> +                 "test_swf_embed_nosound.html";

This breaks the test. Please add the equal sign.

Comment 24

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Comment on attachment 749007 [details] [diff] [review]
> const baseurl,testdata update (master) rev1.2
> 
> ::: mutt/mutt/tests/js/liveweb_tests/flash-protectedmode.js
> 
> This breaks the test. Please add the equal sign.

Thanks, revised patch coming up.

Comment 25

[Jonathan French (:jfrench)]

Attachment: const baseurl,testdata update (master) rev1.3

Patch "baseurl,testdata update (master) rev1.3" for Master. Twenty seven files modified.

This work comprises correction from code review Comment#23 for flash-protectedmode.js. It now passes as expected.

Tested with Default Nightly 23.0a1 20130513031057.

Comment 26

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

Comment on attachment 749241 [details] [diff] [review]
const baseurl,testdata update (master) rev1.3

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

Thanks Jonathan for the updated patch. It looks like that in the interim other tests have been landed which are not part of the patch yet, e.g. js/controller/window_focus.js. It might also apply to other ones. Could you please do a sync?

Comment 27

[Jonathan French (:jfrench)]

Sure, will do.

Comment 28

[Jonathan French (:jfrench)]

I had a look at js/controller/window_focus.js and Andrei's return that created that file, it looks correct and contains BASE_URL,TEST_DATA.

I did another drill through the mutt/mozmill repo but didn't find any more occurrences of unwanted uses of [URL,url] in the context of this patch.

I also checked all new files introduced into the repo over the last 20 returns, since mid-April. There were no other new files introduced which would fall under this refactor, just Andrei's.

I rebased to the latest master without conflicts, and the patch exported was identical to rev1.3 already uploaded. So based on those things, it appears to me that the existing rev1.3 patch has all the desired changes.

Comment 29

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

Comment on attachment 749241 [details] [diff] [review]
const baseurl,testdata update (master) rev1.3

Ok, thanks for checking. I will re-review it later today.

Comment 30

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

Comment on attachment 749241 [details] [diff] [review]
const baseurl,testdata update (master) rev1.3

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

That looks fine! Nice consistency for our Mutt tests. Thanks!

Comment 31

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

Landed as:
https://github.com/mozilla/mozmill/commit/668bf483301fca4e9b0489c74279f0278cdff028