Closed Bug 1382841 Opened 7 years ago Closed 6 years ago

Drop opening new window hack in mochitest

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla60

Tracking Flags:

Tracking

Status

firefox57

---

wontfix

firefox60

---

fixed

People

(Reporter: hiro, Assigned: birtles)

References

Details

Attachments

(7 files)

Proof of concept - part 1 delete step 7 years ago Brian Birtles (:birtles, Feb 26~ parental leave) 2.87 KB, patch		Details \| Diff \| Splinter Review
Proof of concept - part 2 rename step 7 years ago Brian Birtles (:birtles, Feb 26~ parental leave) 947 bytes, patch		Details \| Diff \| Splinter Review
Bug 1382841 - Remove old test_XXX openers from dom/animation/test/**; 7 years ago Brian Birtles (:birtles, Feb 26~ parental leave) 59 bytes, text/x-review-board-request	hiro : review+	Details
Bug 1382841 - Rename test_XXX_to_rename.html files in dom/animation/test to test_XXX.html; 7 years ago Brian Birtles (:birtles, Feb 26~ parental leave) 59 bytes, text/x-review-board-request	hiro : review+	Details
Bug 1382841 - Rename some test files in dom/animation/test/mozilla to use _ to separate words; 7 years ago Brian Birtles (:birtles, Feb 26~ parental leave) 59 bytes, text/x-review-board-request	hiro : review+	Details
Bug 1382841 - Remove old test_XXX openers that just set the dom.animations-api.core.enabled pref from layout/style/test**; 7 years ago Brian Birtles (:birtles, Feb 26~ parental leave) 59 bytes, text/x-review-board-request	hiro : review+	Details
Bug 1382841 - Rename test_XXX_to_rename.html files in layout/style/test to test_XXX.html; 7 years ago Brian Birtles (:birtles, Feb 26~ parental leave) 59 bytes, text/x-review-board-request	hiro : review+	Details

Hiroyuki Ikezoe (:hiro)

Reporter

Description

•

7 years ago

Once bug 1328830 landed we can set preference values in mochitest.ini. yay!

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 1

•

7 years ago

I started looking into this but I'm having trouble getting the right diff.

Specifically I want to delete test_XXX and rename file_XXX to test_XXX (since file_XXX contains the important history).

However, regardless of whether I do `hg rename --force`, or simply `hg rm` followed by `hg mv` I end up with a diff that shows the test_XXX contents being deleted and then added to file_XXX. If I want this to be recorded as an actual rename it seems I need to do it in two steps (i.e. two changesets): one to delete test_XXX, and one to rename file_XXX to test_XXX.

Further, ideally I suppose that in the first patch we should *also* rename file_XXX to test_XXX_2 so that the tests keep running in the intermediate state.

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 2

•

7 years ago

Attached patch Proof of concept - part 1 delete step — Details — Splinter Review

Assignee: nobody → bbirtles

Status: NEW → ASSIGNED

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 3

•

7 years ago

Attached patch Proof of concept - part 2 rename step — Details — Splinter Review

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Updated

•

7 years ago

Attachment #8899343 - Attachment description: Proof of concept - part 1 rename step → Proof of concept - part 1 delete step

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 4

•

7 years ago

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5345a2e3e7325b5088490033d68e684fd381e24b

Try with pref disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd49471b1073516939734cacdb3174ce4180a7fc

Comment hidden (mozreview-request)

We rename the file_XXX contents to test_XXX_to_rename so that these tests keep
running (so long as they begin with file_ and not test_ the test harness will
refuse to run them). If we rename these files to test_XXX in the same patch
Mercurial will not treat this as a rename and so we will lose the history
associated with file_XXX.

Instead, we rename test_XXX_to_rename to test_XXX in the next patch in this
series and by doing this Mercurial will treat this as two rename operations and
`hg log -f test_XXX` will show the history file_XXX as well.

One minor exception to this occurs due to the fact that we have two naming
conventions:

  (a) For tests in css-animations, css-transitions, style etc.

      Most of these tests should eventually land in web-platform-tests. However,
      in many cases that's not yet possible because, for example, CSS
      Animations 2 does not yet specify the behavior tested by the
      css-animations tests.

      For tests in web-platform-tests we generally separate words using -.
      However, our mochitest running machinery requires that tests begin with
      test_. Hence we name tests: test_abc-def-ghi.html.

  (b) For tests in mozilla

      These tests are never intended to be part of web-platform-tests and
      generally for mochitests we use _ to separate words (hence the test_
      prefix). Hence we name these tests test_abc_def_ghi.html

Now, there are some tests in the 'mozilla' directory that use the (a) naming
scheme instead of (b). In this case, instead of renaming file_xxx-xxx.html to
test_xxx-xxx_to_rename.html in this patch, and then renaming
test_xxx-xxx_to_rename.html to test_xxx-xxx.html in a second patch, we can just
delete test_xxx-xxx.html and rename file_xxx-xxx.html to test_xxx_xxx.html in
the one patch and Mercurial will recognize this as a rename because the file
names don't overlap.

Review commit: https://reviewboard.mozilla.org/r/171496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171496/

Comment hidden (mozreview-request)

See the description from in this changeset's parent which describes why we do
this in two steps.

Review commit: https://reviewboard.mozilla.org/r/171498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171498/

Comment hidden (mozreview-request)

This naming convention is described fully in this changeset's grandparent.

Review commit: https://reviewboard.mozilla.org/r/171500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171500/

Comment hidden (mozreview-request)

As with an earlier patch in this series, we rename the file_* test content files
to test_*_to_rename.html in this patch, and then in a subsequent patch drop the
_to_rename suffix so that we can trace the history of the test_* files back to
their file_* equivalents.

Review commit: https://reviewboard.mozilla.org/r/171502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171502/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/171504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171504/

Hiroyuki Ikezoe (:hiro)

Reporter

Comment 10

•

7 years ago

mozreview-review

Comment on attachment 8900117 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in dom/animation/test to test_XXX.html;

https://reviewboard.mozilla.org/r/171498/#review176624

Attachment #8900117 - Flags: review?(hikezoe) → review+

Hiroyuki Ikezoe (:hiro)

Reporter

Comment 11

•

7 years ago

mozreview-review

Comment on attachment 8900116 [details]
Bug 1382841 - Remove old test_XXX openers from dom/animation/test/**;

https://reviewboard.mozilla.org/r/171496/#review176628

Attachment #8900116 - Flags: review?(hikezoe) → review+

Hiroyuki Ikezoe (:hiro)

Reporter

Comment 12

•

7 years ago

mozreview-review

Comment on attachment 8900118 [details]
Bug 1382841 - Rename some test files in dom/animation/test/mozilla to use _ to separate words;

https://reviewboard.mozilla.org/r/171500/#review176630

Attachment #8900118 - Flags: review?(hikezoe) → review+

Hiroyuki Ikezoe (:hiro)

Reporter

Comment 13

•

7 years ago

mozreview-review

Comment on attachment 8900119 [details]
Bug 1382841 - Remove old test_XXX openers that just set the dom.animations-api.core.enabled pref from layout/style/test**;

https://reviewboard.mozilla.org/r/171502/#review176632

Attachment #8900119 - Flags: review?(hikezoe) → review+

Hiroyuki Ikezoe (:hiro)

Reporter

Comment 14

•

7 years ago

mozreview-review

Comment on attachment 8900120 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in layout/style/test to test_XXX.html;

https://reviewboard.mozilla.org/r/171504/#review176634

Attachment #8900120 - Flags: review?(hikezoe) → review+

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 15

•

7 years ago

I've yet to land this because it makes bug 1339648 perma-fail.

Depends on: 1339648

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Updated

•

7 years ago

Depends on: 1393326

Comment hidden (mozreview-request)

Comment on attachment 8900116 [details]
Bug 1382841 - Remove old test_XXX openers from dom/animation/test/**;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171496/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8900117 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in dom/animation/test to test_XXX.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171498/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8900118 [details]
Bug 1382841 - Rename some test files in dom/animation/test/mozilla to use _ to separate words;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171500/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8900119 [details]
Bug 1382841 - Remove old test_XXX openers that just set the dom.animations-api.core.enabled pref from layout/style/test**;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171502/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8900120 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in layout/style/test to test_XXX.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171504/diff/1-2/

Jet Villegas (inactive)

Updated

•

7 years ago

Priority: -- → P3

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Updated

•

7 years ago

status-firefox57: --- → wontfix

Comment hidden (mozreview-request)

Comment on attachment 8900116 [details]
Bug 1382841 - Remove old test_XXX openers from dom/animation/test/**;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171496/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8900117 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in dom/animation/test to test_XXX.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171498/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8900118 [details]
Bug 1382841 - Rename some test files in dom/animation/test/mozilla to use _ to separate words;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171500/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8900119 [details]
Bug 1382841 - Remove old test_XXX openers that just set the dom.animations-api.core.enabled pref from layout/style/test**;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171502/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8900120 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in layout/style/test to test_XXX.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171504/diff/2-3/

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 26

•

6 years ago

Now that bug 1393326 has been resolved, I've unbitrotted this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bae4247246f4b117627803fd36fa6ff4130576d

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 27

•

6 years ago

Looks like test_deferred_start.html needs the opener for timing :/

Comment hidden (mozreview-request)

Comment on attachment 8900116 [details]
Bug 1382841 - Remove old test_XXX openers from dom/animation/test/**;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171496/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8900117 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in dom/animation/test to test_XXX.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171498/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8900118 [details]
Bug 1382841 - Rename some test files in dom/animation/test/mozilla to use _ to separate words;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171500/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8900119 [details]
Bug 1382841 - Remove old test_XXX openers that just set the dom.animations-api.core.enabled pref from layout/style/test**;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171502/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8900120 [details]
Bug 1382841 - Rename test_XXX_to_rename.html files in layout/style/test to test_XXX.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171504/diff/3-4/

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 33

•

6 years ago

Even when I drop the change to test_deferred_start.html it fails.

Depends on: 1419226

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 34

•

6 years ago

What's more, all the tests that rely on prefs still seem to fail on Android.

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 35

•

6 years ago

Perhaps the artifact build didn't include bug 1393326?

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 36

•

6 years ago

Updated try run based on top of the first patch from bug 1419226:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85e3054f93d23fc83ed136edf5da99792009d547

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 37

•

6 years ago

test_deferred_start.html still times out without the opener it seems.

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 38

•

6 years ago

(Actually I made a mistake when I restored the opener for test_deferred_start.html)

It seems to pass with the opener:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3391e11cfe9932d45419261ea14a177a47ab5304

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 39

•

6 years ago

I noticed that bug 1444177 failed twice (out of about ~40 runs) in the above try run.

It's possible this bug slightly increases the likelihood of that intermittent failing simply by some indirect means (e.g. affecting the load of the machine etc.). If that's the case, we should disable that subtest while we investigate it so I think it's ok to land this now.

Pulsebot

Comment 40

•

6 years ago

Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7582d9f9c6e3
Remove old test_XXX openers from dom/animation/test/**; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/965de8e8fcca
Rename test_XXX_to_rename.html files in dom/animation/test to test_XXX.html; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e2ee0c603b
Rename some test files in dom/animation/test/mozilla to use _ to separate words; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d5430e77dc
Remove old test_XXX openers that just set the dom.animations-api.core.enabled pref from layout/style/test**; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ccaacd9692
Rename test_XXX_to_rename.html files in layout/style/test to test_XXX.html; r=hiro

Bogdan Tara[:bogdan_tara | bogdant]

Comment 41

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/7582d9f9c6e3
https://hg.mozilla.org/mozilla-central/rev/965de8e8fcca
https://hg.mozilla.org/mozilla-central/rev/f6e2ee0c603b
https://hg.mozilla.org/mozilla-central/rev/41d5430e77dc
https://hg.mozilla.org/mozilla-central/rev/d6ccaacd9692

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox60: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla60

You need to log in before you can comment on or make changes to this bug.