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

Fix early return inside for loop #66

Merged
merged 2 commits into from
Nov 1, 2019
Merged

Fix early return inside for loop #66

merged 2 commits into from
Nov 1, 2019

Conversation

papandreou
Copy link
Collaborator

Context: https://gitter.im/assetgraph/assetgraph?at=5dbb6438a3f0b17849c488cf

The bug is that we exit out of subsetFonts as soon as we encounter one page without any subset font usages. However, we're in a for loop, so we want to proceed with the remaining pages instead :)

@papandreou papandreou self-assigned this Oct 31, 2019
@coveralls
Copy link

coveralls commented Nov 1, 2019

Pull Request Test Coverage Report for Build 205

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.3%) to 89.064%

Files with Coverage Reduction New Missed Lines %
lib/subsetFonts.js 1 96.67%
lib/HeadlessBrowser.js 2 91.43%
Totals Coverage Status
Change from base Build 193: -0.3%
Covered Lines: 1914
Relevant Lines: 2052

💛 - Coveralls

Copy link
Owner

@Munter Munter left a comment

Choose a reason for hiding this comment

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

Awesome. We probably need to lock this one down with a test, since it was so surprising

@papandreou
Copy link
Collaborator Author

I think it's unlikely to re-occur in this exact form, but I added a regression test now. Should also reclaim the lost coverage 😼

@Munter
Copy link
Owner

Munter commented Nov 1, 2019

Awesone

@papandreou papandreou merged commit dec035a into master Nov 1, 2019
@papandreou papandreou deleted the fix/earlyReturn branch November 1, 2019 23:30
@Munter
Copy link
Owner

Munter commented Nov 2, 2019

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.

3 participants