Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add test for MathJax/MathJax#912 #3

Merged
merged 3 commits into from
Jan 15, 2015
Merged

add test for MathJax/MathJax#912 #3

merged 3 commits into from
Jan 15, 2015

Conversation

pkra
Copy link
Contributor

@pkra pkra commented Jan 15, 2015

No description provided.

@pkra
Copy link
Contributor Author

pkra commented Jan 15, 2015

Not sure if that's what you had in mind. Should I split it into two tests?

try {
check = ( mstyle.getAttribute('scriptlevel') === '0' && mstyle.getAttribute('displaystyle') === 'false');
}
catch (err) {check = false};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but you should be able to do it without the overhead of try/catch. How about:

var check = !!(mstyle && mstyle.getAttribute('scriptlevel') === '0' && mstyle.getAttribute('displaystyle') === 'false');

@dpvc
Copy link
Member

dpvc commented Jan 15, 2015

I think one test is fine for this. If you want, you could do

        return [
          newScriptReftestResult("Scriplevel 0",(mstyle && mstyle.getAttribute('scriptlevel') === '0')),
          newScriptReftestResult("Displaystyle false",(mstyle && mstyle.getAttribute('displaystyle') === 'false'))
        ];

which would be one test file with each part tested separately.

catch (err) {check = false};
return [
newScriptReftestResult("Check displaystyle and scriptlevel changes in toMathML() output",
check),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want the final comma, since that will fail in IE < 9.

@pkra
Copy link
Contributor Author

pkra commented Jan 15, 2015

How about now?

@dpvc
Copy link
Member

dpvc commented Jan 15, 2015

Looks good. The copyright date could be changed and scriptlevel and displaystyle shouldn't be capitalized now that they are not the beginning of the sentence, but I wouldn't worry about those. I'll merge...

dpvc added a commit that referenced this pull request Jan 15, 2015
@dpvc dpvc merged commit 9359a6e into master Jan 15, 2015
@dpvc dpvc deleted the issue912 branch January 16, 2015 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants