sinonjs / formatio

The cheesy object formatter
https://sinonjs.github.io/formatio/
Other
36 stars 11 forks source link

Feature detect Object.getOwnPropertySymbols #28

Closed mantoni closed 5 years ago

mantoni commented 5 years ago

Purpose (TL;DR) - mandatory

This fixes an IE 11 regression introduced in #26. Object.getOwnPropertySymbols is not supported in all environments.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm t

Checklist for author

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 108


Files with Coverage Reduction New Missed Lines %
formatio.js 8 82.24%
<!-- Total: 8 -->
Totals Coverage Status
Change from base Build 105: -0.2%
Covered Lines: 95
Relevant Lines: 113

💛 - Coveralls
BrandonE commented 5 years ago

I have the same issue with code coverage on my PR now. What is your suggestion for handling this, @mantoni? Should we actually use sinon to stub getOwnPropertySymbols to undefined for this test case?

mantoni commented 5 years ago

No. That'd be cheating just for the statistics. If we want to increase coverage for these cases, we'd have to use Mochify and run tests in headless chrome and generate the coverage report there instead of on node.

BrandonE commented 5 years ago

Do you recommend using Mochify or changing the threshold? I plan on applying whatever change you make here to my PR.

mantoni commented 5 years ago

We don't have a treshold configured. It just marks any coverage decrese as a potential issue, I think. So I'm not directly planning to do anything about it 😄

BrandonE commented 5 years ago

Currently, the build checks are failing. Is the plan to override them and just merge?

mantoni commented 5 years ago

The build checks don't prevent merging. I see coverage information as hint. If it drops dramatically, there's something wrong. It's not a hard requirement to always keep the previous value up.

mantoni commented 5 years ago

Released in v3.2.1.