Reuse lengthPairType for positionType in web animations' web-platform-tests

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

We'll add a new type "lengthPairType" in Bug 1354437. Since the existing positionType is similar to this lengthPairType, we shall just reuse it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8863114 [details]
Bug 1359786 - update MANIFEST for web-platform-tests.

https://reviewboard.mozilla.org/r/134942/#review137886

TBH, I'm not expecting the MANIFEST would be changed in this bug. However, after I ran --manifest-update, some diffs indeed showed up. So, I just include the diffs in this separate patch, cause IIRC, we shall always keep the MANIFEST up-to-date.
Attachment #8863112 - Flags: review?(hikezoe)
Attachment #8863113 - Flags: review?(hikezoe)
Attachment #8863114 - Flags: review?(hikezoe)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8863112 [details]
Bug 1359786 - only check the mid-point while testing interpolation results in animations' web-platform-tests.

https://reviewboard.mozilla.org/r/134938/#review137888

Thanks for doing this!
Attachment #8863112 - Flags: review?(hikezoe) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8863113 [details]
Bug 1359786 - reuse existing lengthPairType for positionType in web animations' web-platform-tests.

https://reviewboard.mozilla.org/r/134940/#review137890
Attachment #8863113 - Flags: review?(hikezoe) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8863114 [details]
Bug 1359786 - update MANIFEST for web-platform-tests.

https://reviewboard.mozilla.org/r/134942/#review137892
Attachment #8863114 - Flags: review?(hikezoe) → review+
Btw, you can specify '-u web-platform-tests --artifact' for try to run only web-platform-tests without building for this kind of cases.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Btw, you can specify '-u web-platform-tests --artifact' for try to run only
> web-platform-tests without building for this kind of cases.

Looks like we got 4 unexpected pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae1fc7b720239644bb269006a9f2e4a920ecd499&selectedJob=95430643

So, I'm pushing a separated patch to re-enable these tests. Once the patch is merged to m-c, maybe we can resolve Bug 1291187 as well.

Thank you for the pointer. Since I need to push another round for the additional 4th patch, I'll push with this specific syntax.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8863130 - Flags: review?(hikezoe)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #9)
> So, I'm pushing a separated patch to re-enable these tests. Once the patch
> is merged to m-c, maybe we can resolve Bug 1291187 as well.

Double checked that the issue of Bug 1291187 still remains... not sure why we got these tests unexpected-pass...
Hi hiro, any thoughts? Do you think re-enabling these tests is fine?
Flags: needinfo?(hikezoe)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8863130 [details]
Bug 1359786 - re-enable interpolation tests for word-spacing and flex-basis.

https://reviewboard.mozilla.org/r/134962/#review137908

This chage is fine to me.
I think the problem does not happen when the computed value is calculated through in StyleAnimationValue::AddWeighted() (i.e not at offset 0 or 1).  So it's probably just a serialization problem (I mean it's not an animation problem).
A bad sideeffect of this change what I can think of is that it's hard to notice if someone fix bug 1291187 unintentionally.  So it would be good to add an automation test for it. We have already such test cases somewhere in layout/style/test?

Ah, also it would be nice to check these test cases on stylo.
Attachment #8863130 - Flags: review?(hikezoe) → review+
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Comment on attachment 8863130 [details]
> Bug 1359786 - re-enable interpolation tests for word-spacing and flex-basis.
> 
> https://reviewboard.mozilla.org/r/134962/#review137908
> 
> This chage is fine to me.
> I think the problem does not happen when the computed value is calculated
> through in StyleAnimationValue::AddWeighted() (i.e not at offset 0 or 1). 
> So it's probably just a serialization problem (I mean it's not an animation
> problem).
> A bad sideeffect of this change what I can think of is that it's hard to
> notice if someone fix bug 1291187 unintentionally.  So it would be good to
> add an automation test for it. We have already such test cases somewhere in
> layout/style/test?

Fair enough. Filed Bug 1360929 for this.

> Ah, also it would be nice to check these test cases on stylo.

Thanks for the notice.
I just checked that all these four tests could pass on stylo perfectly. \o/

Comment 18

2 years ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9447fa866d
only check the mid-point while testing interpolation results in animations' web-platform-tests. r=hiro DONTBUILD (test-only)
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b49cc74621
reuse existing lengthPairType for positionType in web animations' web-platform-tests. r=hiro DONTBUILD (test-only)
https://hg.mozilla.org/integration/mozilla-inbound/rev/076b7dd00036
update MANIFEST for web-platform-tests. r=hiro DONTBUILD (test-only)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2793e0dbd4f
re-enable interpolation tests for word-spacing and flex-basis. r=hiro DONTBUILD (test-only)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #17)

> > Ah, also it would be nice to check these test cases on stylo.
> 
> Thanks for the notice.
> I just checked that all these four tests could pass on stylo perfectly. \o/

Yay! Thanks for the check!
(In reply to Pulsebot from comment #18)
> Pushed by jichen@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9447fa866d
> only check the mid-point while testing interpolation results in animations'
> web-platform-tests. r=hiro DONTBUILD (test-only)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/18b49cc74621
> reuse existing lengthPairType for positionType in web animations'
> web-platform-tests. r=hiro DONTBUILD (test-only)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/076b7dd00036
> update MANIFEST for web-platform-tests. r=hiro DONTBUILD (test-only)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c2793e0dbd4f
> re-enable interpolation tests for word-spacing and flex-basis. r=hiro
> DONTBUILD (test-only)

BTW, they are not good uses of DONTBUILD, I should've avoid using this keyword...:(
You need to log in before you can comment on or make changes to this bug.