Open Bug 474742 Opened 13 years ago Updated 10 years ago

SVG SMIL: Convert unit tests to mochitests

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

ASSIGNED

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(7 files, 1 obsolete file)

Attached file 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.
Attached file TestTimeValueSpec.cpp
Attachment #358127 - Attachment description: TestTimedElement → TestTimedElement.cpp
Attachment #358130 - Attachment description: TestTimeVal (reference only -- don't need to convert) → TestTimeVal.h (reference only -- don't need to convert)
Depends on: 216462
Assignee: nobody → birtles
Convert the tests in TestTimeValueSpec.cpp to mochitests. This patch builds on the refactoring of test_smilTiming.xhtml done in bug 705236.
Attachment #599844 - Flags: review?(dholbert)
Status: NEW → ASSIGNED
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)
(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.)
(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).
Attachment #599844 - Attachment is obsolete: true
Attachment #599872 - Flags: review?(dholbert)
Attachment #599844 - Flags: review?(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) :)
Attachment #599872 - Flags: review?(dholbert) → review+
(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)
(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.
Whiteboard: [inbound, part 1]
You need to log in before you can comment on or make changes to this bug.