Skip to content

mocha: non-existent fonts in OS report as pending - #226

Merged
devongovett merged 2 commits into
foliojs:masterfrom
liborm85:test-skip
May 20, 2022
Merged

mocha: non-existent fonts in OS report as pending#226
devongovett merged 2 commits into
foliojs:masterfrom
liborm85:test-skip

Conversation

@liborm85

@liborm85liborm85 commented May 1, 2020

Copy link
Copy Markdown
Contributor

Some tests without font file availability in OS have been marked as passing, even if they were skipped.
Now tests are marked as pending.

Comment threadtest/subset.js

it('should re-encode variation glyphs', function(done) {
if (!fs.existsSync('/Library/Fonts/Skia.ttf')) return done();
if (!fs.existsSync('/Library/Fonts/Skia.ttf')) return this.skip();

@PomaxPomaxMay 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

surely this should be an error, to be thrown instead, through an assertion after openSync that reports "font not found"? Skipping tests because the font files don't exist effectively introduces potential bugs as a side effect of (accidentally) removing a font

@liborm85liborm85May 2, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, this PR is just addresses to solve potential bugs about which you writes. These are fonts that are not available in the repository for legal reasons. They are only in filesystem on OS, especially on MacOS.
If someone does not have these fonts, tests cannot fail with error message "font not found", they would never pass the tests and this is wrong behavior. Previously without existence of a file, tests passed "silently". Now is skipped it and is reported to user. This is the expected correct behavior in mocha tests.

@liborm85liborm85 changed the title mocha: non-existent fonts report as pendingmocha: non-existent fonts in OS report as pendingMay 2, 2020
@devongovett
devongovett merged commit 13e904a into foliojs:masterMay 20, 2022
Sign up for freeto 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.

3 participants

@liborm85@Pomax@devongovett