Make Mozmill-test testBackForwardButtons local

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: ashughes, Assigned: aaronmt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [litmus-data])

Attachments

(5 attachments)

(Reporter)

Description

8 years ago
Module: testGeneral/testBackForwardButtons.js
Test-page: Use any 3 pages from test-files/layout/mozilla*

NOTE: Please move test module from testGeneral to testToolbar
(Reporter)

Updated

8 years ago
Blocks: 579965
(Assignee)

Comment 1

8 years ago
Created attachment 466709 [details] [diff] [review]
Patch v1 - (default)

test move to firefox/testToolbar (as requested in comment #0) + cleanup + converting test to local-data
Assignee: nobody → aaron.train
Status: NEW → ASSIGNED
Attachment #466709 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 2

8 years ago
Comment on attachment 466709 [details] [diff] [review]
Patch v1 - (default)

Looks good, r+.  Over to Henrik for additional review.
Attachment #466709 - Flags: review?(hskupin)
Attachment #466709 - Flags: review?(anthony.s.hughes)
Attachment #466709 - Flags: review+
Comment on attachment 466709 [details] [diff] [review]
Patch v1 - (default)

First, please avoid moving of files and changes at the same time. It makes the review too complex. Instead handle those kind of tasks separately.

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
>+const LOCAL_TEST_PAGES = [
>+  {url: LOCAL_TEST_FOLDER + 'layout/mozilla.html', id: 'community'},
>+  {url: LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html', id: 'mission_statement'}
>+];

I would like to see three pages here as what we had in the original code. That way we can add more tests to this function to check the enabled/disabled states for both buttons. Would be good to have in any way.

>+var testBackandForward = function()
>+{

nit: Please fix the bracing.
Attachment #466709 - Flags: review?(hskupin) → review-
(Assignee)

Comment 4

8 years ago
Created attachment 471573 [details] [diff] [review]
Patch v1.1 - (default)

Thanks for catching the two as opposed to three test files! Fixes as per comment #3
Attachment #471573 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 5

8 years ago
Comment on attachment 471573 [details] [diff] [review]
Patch v1.1 - (default)

2 things:

1. Please rename testBackandForward to testBackAndForward()
2. As Henrik already asked, please do the move and the revisions in separate patches

Thanks!
Attachment #471573 - Flags: review?(anthony.s.hughes) → review-
(Assignee)

Comment 6

8 years ago
1. Ok.
2. Since he already reviewed it, there shouldn't be a need. I'll do so accordingly in the future.
(Reporter)

Comment 7

8 years ago
(In reply to comment #6)
> 1. Ok.
> 2. Since he already reviewed it, there shouldn't be a need. I'll do so
> accordingly in the future.

Henrik, do you want this broken into 2 patches or is it ok in a single patch this time?
It's fine for this time. Just keep an eye on it in the future.
(Assignee)

Comment 9

8 years ago
Created attachment 471620 [details] [diff] [review]
Patch v1.2 - (default)

Renamed testBackandForward() to testBackAndForward()
Attachment #471620 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 10

8 years ago
Comment on attachment 471620 [details] [diff] [review]
Patch v1.2 - (default)

r+, assuming you don't need to re-review this Henrik.
Attachment #471620 - Flags: review?(anthony.s.hughes) → review+
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 11

8 years ago
(In reply to comment #10)
> Comment on attachment 471620 [details] [diff] [review]
> Patch v1.2 - (default)
> 
> r+, assuming you don't need to re-review this Henrik.

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/1ed816089b1e

Please followup with 1.9.2 and 1.9.1 patches as appropriate.
(Assignee)

Comment 12

8 years ago
Created attachment 473620 [details] [diff] [review]
Patch v1.2 - (1.9.2)
Attachment #473620 - Flags: review?(anthony.s.hughes)
(Reporter)

Updated

8 years ago
Attachment #473620 - Flags: review?(anthony.s.hughes) → review+
(Reporter)

Comment 13

8 years ago
(In reply to comment #12)
> Created attachment 473620 [details] [diff] [review]
> Patch v1.2 - (1.9.2)

Landed on mozilla1.9.2:
http://hg.mozilla.org/qa/mozmill-tests/rev/8e80d946710b
Keywords: checkin-needed
(Assignee)

Comment 14

8 years ago
Created attachment 474210 [details] [diff] [review]
Patch v1.2 - (1.9.1)
(Reporter)

Updated

7 years ago
Keywords: checkin-needed
(Reporter)

Comment 15

7 years ago
(In reply to comment #14)
> Created attachment 474210 [details] [diff] [review]
> Patch v1.2 - (1.9.1)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/81cb146ae2f3 [mozilla1.9.1]
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
You need to log in before you can comment on or make changes to this bug.