963324 - layout/reftests/mathml/menclose-1-ref.html tests the wrong feature

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #963198 +++

When the UPDIAGONALARROW notation was added to the <menclose> feature in bug 875294 the test checked in with it used the wrong notation, updiagonalstrike, which we already have tests for.

We need to update layout/reftests/mathml/menclose-1-ref.html to use the intended "updiagonalarrow" notation, which would have caught bug 963198 when we turned on TBPL ASAN tests.

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Import mismatch menclose reftests from the MathJax testsuite

Add some tests from the MathJax test suite. Updiagonalarrow is tested by menclose-1p.html. The tests are only checking that notation="" renders differently than notation="SOMENOTATION". I only ran them locally, not on the try server, as I don't know what is the policy regarding secured bugs.

I think it would be worth making the fuzzer import tests from https://github.com/mathjax/MathJax-test/tree/master/testsuite/MathMLToDisplay/ if that's not already the case.

I have also opened bug 963453.

Comment 2

[Frédéric Wang (:fredw)]

Comment on attachment 8364897 [details] [diff] [review]
Import mismatch menclose reftests from the MathJax testsuite

Asking review to Karl for when he is back.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 8364897 [details] [diff] [review]
Import mismatch menclose reftests from the MathJax testsuite

Review of attachment 8364897 [details] [diff] [review]:
-----------------------------------------------------------------

Karl's review is sufficient. I know enough to notice it didn't test what it claimed but otherwise don't know MathML.

Comment 4

[Frédéric Wang (:fredw)]

Are we waiting any security approval before landing this patch? These are just reftests so they only need to land in trunk. I'm asking that because some volunteers will work on menclose reftests (bug 914031 and probably bug 963453 too).

@Jesse: did you integrate https://github.com/mathjax/MathJax-test/tree/master/testsuite/MathMLToDisplay/ in your fuzzer as suggested in comment 1?

Comment 5

[Al Billings [:abillings - ex-MoCo]]

This is a sec-other and doesn't need security approval to land though no one has asked on the patch.

Bug 963198 was checked in and fixed so they should just land.

Comment 6

[Frédéric Wang (:fredw)]

Thanks for the information.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e386c02b4b9a

Comment 8

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/1ca0ce406aad for timeouts like https://tbpl.mozilla.org/php/getParsedLog.php?id=34256587&tree=Mozilla-Inbound

Comment 9

[Frédéric Wang (:fredw)]

Attachment: Patch V2

Comment 10

[Jesse Ruderman]

I haven't integrated the MathJAX test suite yet, but it's on my list.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a4562d20cb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97a4562d20cb

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Frédéric Wang (:fredw) from comment #4)
> These are just reftests so they only need to land in trunk.

Any particular reason for that? Seems like the best way to verify a fix on a branch is to land a test for it :)

Comment 14

[Frédéric Wang (:fredw)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #13)
> (In reply to Frédéric Wang (:fredw) from comment #4)
> > These are just reftests so they only need to land in trunk.
> 
> Any particular reason for that? Seems like the best way to verify a fix on a
> branch is to land a test for it :)

I just meant that only the code change was necessary to fix the security bug, but please ignore my comment and follow your usual procedures. (Also, the tests do not really verify the fix but only gives testcases that the fuzzer could have used to detect the issue...)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9449a68dd30d
https://hg.mozilla.org/releases/mozilla-beta/rev/c6718f474d3f
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4fd893896d02

I'll land this on esr24 as a ride-along the next time I'm pushing there.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/c6718f474d3f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/5e1874d6b7ea