849986 - Convert occurrences of external http or ftp references, to TEST_DATA

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Jonathan French (:jfrench)]

Begat from Comment#11 in bug 848649:

> 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.

So this bug entered, is for the work to convert TEST_PAGE, to EXTERNAL_URL, for clarity. Feel free to assign me and I'll be happy to make the changes, when it's the right time to do so. See also bug 849962(moving remote testcases to remote testrun) which I don't think is a blocker but is related.

Comment 1

[Jonathan French (:jfrench)]

Noting these here also, just in case we want to do anything about these when we update TEST_PAGE to EXTERNAL_URL. 

We have a variety of AddOns which we could reference the same way, so in theory we could reliably find every external reference in the repo.

tests\functional\restartTests\testAddons_installFromFTP
const ADDON = [
  {id: "[e-mail]",
   url: "ftp://ftp.mozqa.com/data/firefox/addons/extensions/empty.xpi"}
];

eg.
const ADDON
to
const EXTERNAL_ADDON

Then grep'ing for EXTERNAL, would find everything, with the full strings for each returning just the subsets.

Though perhaps ADDON is already self evident.. and people just have to remember they exist if they are searching for external references.

Comment 2

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

So I slept over it by making a bit more thoughts about such a change. Not sure if I'm really happy with it in terms of destroying consistency through tests vs. making it clear where the test is coming from. As of now I would vote that we stick with TEST_URL whether if its local or remote. It will help people to directly detect that this constant is about an URL for a web page. EXTERNAL_URL instead could be anything.

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.

What do you think? Also cc'ing Dave for his input.

Comment 3

[Jonathan French (:jfrench)]

Sure, sounds good to me. You could find either class of test that way with a pattern search through the repo. I defer to you guys though if you need to tweak the concept further.

Related, I started in on bug 848649 (BASE_URL,TEST_URL), am about 25% of the way through the files at the moment.

Comment 4

[Jonathan French (:jfrench)]

A note that some of the conversions for external test urls to TEST_URL, have occurred as a by product of work in bug 848361. There may still be more, so this bug is still reasonable to leave open at this time representing the remote work.

Comment 5

[Jonathan French (:jfrench)]

Attachment: external testurl update (default) - wip - rev1.0

Temporary work-in-progress patch, wip rev 1.0.

Since bug 848649 is well under way for Default, and it looks like we're proceeding in some form on this, I did a first pass converting as per Henrik's current thoughts. I can always change it if needed.

No review required at this point, just storing the work remotely here as a backup.

A couple notes for my own reference

lib/addons contains an external reference in AMO_PREVIEW_DOMAIN which has been left unchanged for now, and a number of files' urls exceed 80 characters(in addition to many other lines in the files). However, the >80char TEST_URL could be split in some way if required.

I also need to do some more drilling through the repo to see if I can find any others lurking around used in other, different ways.

Comment 6

[Jonathan French (:jfrench)]

Attachment: external testurl update (default) - wip - rev1.1

Earlier work in progress patch just uploaded, had lib/downloads involved, and even though it referenced an external url I believe it should remain as is. Otherwise the same comments apply as in rev1.0 above.

Comment 7

[Jonathan French (:jfrench)]

Actually, I was crossing lib/downloads(which also has external stuff in it) and lib/tests/testDownloads. The latter actually can be in the patch, and was. So my original 'wip - rev1.0' patch as a start was reasonable and I'm going to switch the obsolete flags accordingly.

Comment 8

[Jonathan French (:jfrench)]

We may use a value other than TEST_URL, pending the work in bug 848649.

Comment 9

[Jonathan French (:jfrench)]

We have almost landed the patch for bug 848649 on default, and we are going with TEST_DATA instead of TEST_URL,TEST_URL(S). So I will begin updating this patch to reflect that approach.

Comment 10

[Jonathan French (:jfrench)]

Just noting here a couple of external url references I don't intend to touch at this time. There were other candidates to investigate, but I've excluded all the other cases without needing to note them.

The first; key,value pairs which reference external url's. They could be adjusted in a separate bug if desired. This one was mentioned in Comment#1 in a separate context.

eg. (tests/functional/restartTests/testAddons_installFromFTP/test1.js)
  {id: "[e-mail]",
   url: "ftp://ftp.mozqa.com/data/firefox/addons/extensions/empty.xpi"}
];


The second, sort of an unusual use, and in a lib. Will not touch this either, also as mentioned in Comment#5.

lib/addons.js
const AMO_PREVIEW_SITE = "https://" + AMO_PREVIEW_DOMAIN;

Comment 11

[Jonathan French (:jfrench)]

Attachment: external testurl update (default) rev1.0

Patch "external testurl update (default) rev1.0" for the Default branch. Eighteen files modified.

This work represents the conversion to TEST_DATA, for all tests referencing external, (presently)non-relocatable, urls.

In some cases minor adjustments were made. One assert message was tweaked. In the case of testSafeBrowsingNotificationBar, a for-loop converted to for-each, to allow for hopefully cleaner code, and because it didn't require an incrementing loop. 

Some single line comments were added for new TEST_DATA arrays, which previously were discretely named constants. This should ease any maintenance of those tests.

(assert message tweak)
restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test2

(forloop) testSecurity/testSafeBrowsingNotificationBar

In some cases, exceptionally long urls were broken into two lines where they exceeded ~90 characters, while being kept readable. Most were preserved intact, where possible.

Tested with Default Nightly 23.0a1 20130506030925. Tests pass where expected, and produce the same result as current Default minus the patch.

Comment 12

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

Comment on attachment 746074 [details] [diff] [review]
external testurl update (default) rev1.0

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

Looks great and I cannot see test failures. It's good for landing. Thank you Jonathan!

Comment 13

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

http://hg.mozilla.org/qa/mozmill-tests/rev/164e7661817f (default)