474742 - SVG SMIL: Convert unit tests to mochitests

Status: ASSIGNED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: TestTimedElement.cpp

There are several unit tests for SMIL written as C++ unit tests that ideally we would like to include as mochitests but first they need to be converted to a suitable form.

I have begun this process and posted the first results under bug 474739. The test cases attached to that bug represent the first part of TestTimedElement.cpp up to TestEndSpecs().

The relevant C++ files are attached.

Comment 1

[Brian Birtles (:birtles)]

Attachment: TestAnimationFunction.cpp

Comment 2

[Brian Birtles (:birtles)]

Attachment: TestTimeValueSpec.cpp

Comment 3

[Brian Birtles (:birtles)]

Attachment: TestTimeVal.h (reference only -- don't need to convert)

Comment 4

[Brian Birtles (:birtles)]

Attachment: MockAnimAttr.h (reference only -- don't need to convert)

Comment 5

[Brian Birtles (:birtles)]

Attachment: MockAnimVal.h (reference only -- don't need to convert)

Comment 6

[Brian Birtles (:birtles)]

Attachment: Part 1 - Convert tests in TestTimeValueSpec.cpp to mochitests

Convert the tests in TestTimeValueSpec.cpp to mochitests. This patch builds on the refactoring of test_smilTiming.xhtml done in bug 705236.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 599844 [details] [diff] [review]
Part 1 - Convert tests in TestTimeValueSpec.cpp to mochitests


Assuming you agree with my "constructStartTimeTest" suggestion over on bug 705236, this patch here wants s/testStartTime/constructStartTimeTest/ (or whatever you end up naming the function -- maybe "makeStartTimeTest" to be more frugal with characters, given that this patch has some long lines. :))

>+++ b/content/smil/test/test_smilTiming.xhtml
>+  // Offset syntax
>+  // -- Basic tests, sign and whitespace
>+  testCases.push(testStartTime('3s', 3));
>+  testCases.push(testStartTime('3s', 3));

I think those^ are identical?

>+  testCases.push(testStartTime('+\n5s', 5));

Add one of these for (-\n5s', -5), too.

>+  testCases.push(testStartTime('02:59.999999999999999999999',
>+      2*secPerMin + 60));

Why not just 3*secPerMin?

>+  testCases.push(testStartTime('0.999999999999999999999999999999999999999999' +
>+                               '99999999999999999999999999999999999999', 1));

I don't understand this one -- it looks like it's saying 99999999.9999 = 1?  (with more 9's)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> >+  testCases.push(testStartTime('+\n5s', 5));
> 
> Add one of these for (-\n5s', -5), too.

(The point being that we might just happen to get the right answer on "+\n5s" by ignoring stuff before the \n, but a testcase with "-" would catch that sort of bug.)

Comment 9

[Brian Birtles (:birtles)]

Attachment: Part 1 - Convert tests in TestTimeValueSpec.cpp to mochitests v1b

(In reply to Daniel Holbert [:dholbert] from comment #7)
> >+  testCases.push(testStartTime('+\n5s', 5));
> 
> Add one of these for (-\n5s', -5), too.

Good idea, fixed.

> >+  testCases.push(testStartTime('02:59.999999999999999999999',
> >+      2*secPerMin + 60));
> 
> Why not just 3*secPerMin?

Yep, that's better.

> >+  testCases.push(testStartTime('0.999999999999999999999999999999999999999999' +
> >+                               '99999999999999999999999999999999999999', 1));
> 
> I don't understand this one -- it looks like it's saying 99999999.9999 = 1? 
> (with more 9's)

It's string concatenation, not addition, so it's actually 0.9999999999999999999 = 1 (but with more 9's).

I guess the original test was probing for some kind of overflow case because it sure does seem like a lot of 9s (hence why I had to break it up over two lines).

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 599872 [details] [diff] [review]
Part 1 - Convert tests in TestTimeValueSpec.cpp to mochitests v1b

(In reply to Brian Birtles (:birtles) from comment #9)
> > I don't understand this one -- it looks like it's saying 99999999.9999 = 1? 
> > (with more 9's)
> 
> It's string concatenation, not addition, so it's actually
> 0.9999999999999999999 = 1 (but with more 9's).

Ah, right. Do we get any benefit from using that many 9s, though, partiuclarly given that this test explicitly rounds at 3 digits?

I'd think we could replace this with just "0.9999" and call it good. :)

r=me with a simplification like that (or with a reason why we benefit from using so many 9s) :)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> I'd think we could replace this with just "0.9999" and call it good. :)

(or just the single line of 9's, up to the "+" -- the point is, the [[+ '9999...9']] reduces test-readability without having any effect on the test's behavior)

Comment 12

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> (In reply to Daniel Holbert [:dholbert] from comment #10)
> > I'd think we could replace this with just "0.9999" and call it good. :)
> 
> (or just the single line of 9's, up to the "+" -- the point is, the [[+
> '9999...9']] reduces test-readability without having any effect on the
> test's behavior)

Fixed, thanks Daniel!

Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7015541bbbdb

Please don't close this bug on merge to m-c. There's still more work to do here.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7015541bbbdb