Add MPL2.0 and update controller.open() to use constants for testMetro/testTabs.js

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
--
trivial
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jfrench, Assigned: jfrench)

Tracking

Details

(Whiteboard: [mentor=whimboo][lang=js][good first bug][mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=2], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
I noticed in my travels that mutt/mutt/tests/js/metro/testTabs.js is missing its TEST_DATA constant and MPL2.0 block. It's presently using file paths directly in controller.open(). It seems it's been that way since inception.

This bug will make those updates to the test so is consistent with the rest of the repo.

I will hold off on making this change until after the larger structural repo change in bug 767286 is landed by Dave. Which should be very soon anyway.
Whiteboard: [lang=js] → [mentor=whimboo][lang=js][good first bug]
(Assignee)

Comment 1

5 years ago
Created attachment 782371 [details] [diff] [review]
update metro testTabs (master) rev1.0

Patch "update metro testTabs (master) rev1.0" for Master branch. One file modified.

Contains the addition of the MPL2.0, a new TEST_DATA block and its references in controller.open().

Also added a function name which describes what the test does, and a comment directly above it, as a precursor to similar function naming work across the repo in bug 872174. Please let me know what adjustments need to be made if any.

Although I understand we don't want to run on Metro right now, on a non-Metro system like XP the result is a pass and also consistent with latest clean master.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch rev1.0
latest Nightly 25.0a1 20130728030204 140267
Attachment #782371 - Flags: review?(hskupin)
(Assignee)

Comment 2

5 years ago
Comment on attachment 782371 [details] [diff] [review]
update metro testTabs (master) rev1.0

Whimboo is away till 08/11 and Dave is also away (I think), so setting review for Andreea. Andreea if you want me to omit the test name change and its comment addition, let me know and I can provide a rev1.1 patch which just makes the changes in the original scope of the bug. That might be a safer bet for now.
Attachment #782371 - Flags: review?(hskupin) → review?(andreea.matei)
Comment on attachment 782371 [details] [diff] [review]
update metro testTabs (master) rev1.0

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

Looks good, just a small comment. We'd also need Dave here so you can request from him with that thing solved :)

::: mutt/mutt/tests/js/testMetro/testTabs.js
@@ +10,5 @@
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
>  }
>  
> +// Validate tab manipulation under the Metro interface

It's not a big deal, but would you mind to update this as a js doc, with /* comment? That's how we use for all function descriptions.
Attachment #782371 - Flags: review?(andreea.matei) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #3)
> Comment on attachment 782371 [details] [diff] [review]
> > +// Validate tab manipulation under the Metro interface
> 
> It's not a big deal, but would you mind to update this as a js doc, with /*
> comment? That's how we use for all function descriptions.

Right on, thanks for catching that Andreea. Will update it accordingly.
(Assignee)

Comment 5

5 years ago
Created attachment 785044 [details] [diff] [review]
update metro testTabs (master) rev1.1

Patch "update metro testTabs (master) rev1.1" for Master branch. One file modified.

Basically the same patch as Comment 1, but with function comment block adjustment as per Comment 3.

I notice that the Mozilla Style Guide shows a 2 character indent, whereas most of the repo examples I found use a 1 character indent for function comment blocks. I went with the repo.

Unrelated but I noticed that with this patch the new function name nicely gives the user a bit more information on a command line run. Rather than just "test".

TEST-START | testTabs.js | testTabsUnderMetro
TEST-PASS | testTabs.js | testTabsUnderMetro

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch rev1.1
latest Nightly 25.0a1 20130802030204 140971
Attachment #785044 - Flags: review?(dave.hunt)
(Assignee)

Updated

5 years ago
Attachment #782371 - Attachment is obsolete: true
Comment on attachment 785044 [details] [diff] [review]
update metro testTabs (master) rev1.1

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

Looks good, with the mentioned change I think we can land this.

::: mutt/mutt/tests/js/testMetro/testTabs.js
@@ +13,5 @@
>  
> +/**
> + * Validate tab manipulation under the Metro interface
> + */
> +function testTabsUnderMetro() {

Can you name this method 'testMetroTabs' rather than set an 'UnderMetro' precedent. The test file is within the 'testMetro' folder so we could even drop the reference from the name entirely, but I'm happy for it to be there.
Attachment #785044 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 7

5 years ago
Makes sense, and sounds good. Coming up.
(Assignee)

Comment 8

5 years ago
Created attachment 788750 [details] [diff] [review]
update metro testTabs (master) rev1.2

Patch "update metro testTabs (master) rev1.2" for Master branch. One file modified.

Essentially the same patch as Comment 5, but with function name adjustment as per Comment 6.

Re-tested via mutt -m, as metro is absent in the parent js manifest.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch rev1.2
latest Nightly 26.0a1 20130811030225 142101
Attachment #785044 - Attachment is obsolete: true
Attachment #788750 - Flags: review?(dave.hunt)
(Assignee)

Comment 9

5 years ago
Created attachment 788945 [details] [diff] [review]
update metro testTabs (master) rev1.3

Patch "update metro testTabs (master) rev1.3" for Master branch. One file modified.

The same patch as Comment 8, but rebased to latest master. Since we had another return to the repo a short while ago. There were no intersecting files, just to make a patch with a nicer history.
Attachment #788750 - Attachment is obsolete: true
Attachment #788750 - Flags: review?(dave.hunt)
Attachment #788945 - Flags: review?(dave.hunt)
Jonathan, if there is no conflict when applying the patch, there is no need to attach a new version. It will not change anything to the history. :)
(Assignee)

Comment 11

5 years ago
Thanks, ya after attaching I just realized that almost simultaneously. :)
Attachment #788945 - Flags: review?(dave.hunt) → review+
Landed on master:
https://github.com/mozilla/mozmill/commit/b03e9e4bfb2fc6f22d14e1fe59c190cfaae11533
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=whimboo][lang=js][good first bug] → [mentor=whimboo][lang=js][good first bug][mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=2]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.