Closed Bug 975681 Opened 10 years ago Closed 10 years ago

Add tests for MathML mfrac

Categories

(Core :: MathML, defect, P5)

Product:
Core
Component:
MathML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: anujagarwal464, Assigned: anujagarwal464)

References

Details

Attachments

(2 files, 4 obsolete files)

mochitests for MathML mfrac
4.96 KB, patch
fredw
: review+
Details | Diff | Splinter Review
bug975681-reftests.diff
31.35 KB, patch
fredw
: review+
Details | Diff | Splinter Review 
      No description provided.
Blocks: 897065
Priority: -- → P5
Assignee: nobody → anujagarwal464
Attached patch bug975681.diff (obsolete) — — Splinter Review 

        Attachment #8395354 -
        Flags: feedback?(fred.wang)

    
    

    
  
Comment on attachment 8395354 [details] [diff] [review]
bug975681.diff

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

Thanks Anuj. Tests look good, but as I told you for menclose, using == tests is often more accurate than !=. Could you try to see if you could import and adapt some tests from https://github.com/mathjax/MathJax-test/tree/master/testsuite/MathMLToDisplay/Presentation/GeneralLayout/mfrac? I think they could replace many of the mfrac-A* tests.

For dynamic tests, I guess you also want to test settting/adding attributes and adding/removing children, not just attribute removal.

Could you also try to write Mochitests (see layout/mathml/tests) based on the MathML Acid3 (search mfrac in https://github.com/fred-wang/AcidTestsMathML/blob/master/acid3/index.html)?

        Attachment #8395354 -
        Flags: feedback?(fred.wang) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug975681-reftests.diff (obsolete)
        — 
          — Splinter Review
      

    


  
I will add Mochitests in another patch.

        Attachment #8395354 -
        Attachment is obsolete: true

        Attachment #8397008 -
        Flags: feedback?(fred.wang)

    
    

    
  
Comment on attachment 8397008 [details] [diff] [review]
bug975681-reftests.diff

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

Thank you, that looks very good. 

r=me with mfrac-A-6.html removed.

> I will add Mochitests in another patch.

Yes, please upload a separate patch with only Mochitests, so that will be more convenient for review.

::: layout/reftests/mathml/mfrac-A-6.html
@@ +5,5 @@
> +  </head>
> +  <body>
> +    <div style="position: absolute; left: 20px; top: 20px; width: 200px; height: 34px; background: red;"></div>
> +  </body>
> +</html>

I'm not sure this "34px" value is really reliable. I'd prefer a mochitest for that one.

        Attachment #8397008 -
        Flags: review+

        Attachment #8397008 -
        Flags: feedback?(fred.wang)

        Attachment #8397008 -
        Flags: feedback+

    
  
Summary: Add tests for mfrac → Add tests for MathML mfrac

    
    

    
  

      
      
      
      

        Attached patch
          
          
        reftests for MathML mfrac (obsolete)
        — 
          — Splinter Review
      

    


  
updated the patch.

        Attachment #8397008 -
        Attachment is obsolete: true

        Attachment #8397823 -
        Flags: review?(fred.wang)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        mochitests for MathML mfrac (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8397824 -
        Flags: review?(fred.wang)

        Attachment #8397823 -
        Flags: review?(fred.wang) → review+

    
    

    
  
Comment on attachment 8397824 [details] [diff] [review]
mochitests for MathML mfrac

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

Thank you!

r=me with the comments below addressed.

::: layout/mathml/tests/test_bug975681.html
@@ +24,5 @@
> +  </p>
> +  
> +  <p>
> +    <math>
> +      <mfrac linethickness="2em" id="mfrac_linethickness">

I think this should be "px" rather than em so that it won't depend on the font-size. You should probably try a large enough thickness like "30px" so that it is larger than possible measurement errors.

@@ +100,5 @@
> +    var mfrac = document.getElementById("mfrac_linethickness").getBoundingClientRect();
> +    var num = document.getElementById("mfrac_linethickness").firstElementChild.getBoundingClientRect();
> +    var denom = document.getElementById("mfrac_linethickness").lastElementChild.getBoundingClientRect();
> +    
> +    ok(almostLessThanRel(num.height + 2 + denom.height, mfrac.height) && almostLessThanAbs(num.bottom + 2, denom.top), "numerator and denominator should be separated by linethickness")

I think you can just do

almostLessThan(num.height + 30 + denom.height, mfrac.height) and 
almostLessThan(num.bottom + 30 , denom.top)

The additional space between the bar and num/denum will even more help to pass the condition.

Then the *Rel functions can be removed.

        Attachment #8397824 -
        Flags: review?(fred.wang)

        Attachment #8397824 -
        Flags: review+

        Attachment #8397824 -
        Flags: feedback+

    
    

    
  


  
updated tests. Thanks for help! :D

        Attachment #8397824 -
        Attachment is obsolete: true

        Attachment #8397864 -
        Flags: review?(fred.wang)

        Attachment #8397864 -
        Flags: review?(fred.wang) → review+

    
    

    
  
mfrac-C-1 and mfrac-D-1 are failing on OS X [max difference: 1, number of differing pixels: 1]. I think it would be OK to mark them fuzzy.

mfrac-B-1 is failing on windows 7, don't know what to do with this test.
Flags: needinfo?(fred.wang)

    
    

    
  
(In reply to Anuj Agarwal [:anujagarwal464] from comment #10)
> mfrac-C-1 and mfrac-D-1 are failing on OS X [max difference: 1, number of
> differing pixels: 1]. I think it would be OK to mark them fuzzy.

OK.

> 
> mfrac-B-1 is failing on windows 7, don't know what to do with this test.

when I open reftest analyzer, the "B" denominator seems to be slightly shifted. I think using <mspace/> elements for num/denum might help.
Flags: needinfo?(fred.wang)

    
    

    
  


  
updated the mfrac-B-1 test.

        Attachment #8397823 -
        Attachment is obsolete: true

        Attachment #8398532 -
        Flags: review?(fred.wang)

        Attachment #8398532 -
        Flags: review?(fred.wang) → review+

    
    

    
  
(In reply to Frédéric Wang (:fredw) from comment #13)
> https://tbpl.mozilla.org/?tree=Try&rev=44222f065450

All test passed! :D
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/742f05b9917b
https://hg.mozilla.org/mozilla-central/rev/9d0ede143f7c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

    
    

    
  
The following changesets are now in Firefox Nightly:

> 742f05b9917b Bug 975681 - Mochitest for MathML mfrac. r=fredw
> 9d0ede143f7c Bug 975681 - Reftests for MathML mfrac. r=fredw

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: