-
-
Notifications
You must be signed in to change notification settings - Fork 771
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: assertion log limit #2485
fix: assertion log limit #2485
Conversation
6709870
to
82aaaba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage. ✌️
Would you be interested in help getting it over the finish line? Have some time these days. |
Sure that would be great. I lost track of this but think it would be a great add. I can also put some time in to addressing your feedback as well |
The 'assert' object is already present on the sandbox
Co-authored-by: Spencer Goossens <[email protected]> Co-authored-by: Carl-Erik Kopseng <[email protected]>
82aaaba
to
351d630
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2485 +/- ##
==========================================
+ Coverage 96.03% 96.05% +0.02%
==========================================
Files 40 41 +1
Lines 1915 1928 +13
==========================================
+ Hits 1839 1852 +13
Misses 76 76
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
OK, so I added test coverage, fixed up some logical errors, exposed the options on the sandbox creation, etc. This is ready for merge from my perspective, but EDIT: we changed it to a default of 10k |
@sgoossens You ok with this? Anything to add/change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great thank you for helping get this over the line
rather than implementation specific.
Purpose (TL;DR) - mandatory
Allow for a log limit setting on the assertion object, that when true, will truncate the error.message that is returned via a failing assertion. The limit for the log message will be set to a default if none was provided
#2484
Background (Problem in detail) - optional
When a stubbed method has a large property on it, it is logged in the failing assertion causing runoff in the terminal and making it hard to see what assertion actually failed.
By allowing for a log limitation setting and a default log limit, error logs can be truncated.
How to verify - mandatory
Checklist for author
npm run lint
passes