Skip to content

fix: assertion log limit#2485

Merged
fatso83 merged 8 commits into
sinonjs:mainfrom
sgoossens:feat/allow-limit-to-assertion-logs
Oct 27, 2023
Merged

fix: assertion log limit#2485
fatso83 merged 8 commits into
sinonjs:mainfrom
sgoossens:feat/allow-limit-to-assertion-logs

Conversation

@sgoossens

@sgoossens sgoossens commented Dec 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

  1. use this app and point the sinon instance to this version
  2. see the logs are reduced when the setting is on

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@sgoossens
sgoossens force-pushed the feat/allow-limit-to-assertion-logs branch 2 times, most recently from 6709870 to 82aaaba Compare December 12, 2022 21:31
fatso83
fatso83 requested changes Dec 15, 2022
View reviewed changes

@fatso83 fatso83 left a comment

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.

Missing test coverage. ✌️

fatso83
fatso83 reviewed Feb 22, 2023
View reviewed changes
Comment thread lib/sinon/assert.js Outdated
@fatso83

fatso83 commented Aug 9, 2023

Copy link
Copy Markdown
Contributor

Would you be interested in help getting it over the finish line? Have some time these days.

@sgoossens

sgoossens commented Aug 9, 2023

Copy link
Copy Markdown
Contributor Author

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

@fatso83
fatso83 force-pushed the feat/allow-limit-to-assertion-logs branch from 82aaaba to 351d630 Compare October 25, 2023 18:35
@codecov

codecov Bot commented Oct 25, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8dbfd02) 96.03% compared to head (7701afc) 96.05%.
Report is 4 commits behind head on main.

❗ Current head 7701afc differs from pull request most recent head ef5c456. Consider uploading reports for the commit ef5c456 to get more accurate results

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
Flag Coverage Δ
unit 96.05% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lib/create-sinon-api.js 100.00% <100.00%> (ø)
lib/sinon.js 100.00% <100.00%> (ø)
lib/sinon/assert.js 100.00% <100.00%> (ø)
lib/sinon/colorizer.js 100.00% <ø> (ø)
lib/sinon/create-sandbox.js 100.00% <100.00%> (ø)
lib/sinon/sandbox.js 97.90% <100.00%> (+<0.01%) ⬆️
lib/sinon/spy-formatters.js 98.55% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fatso83 fatso83 mentioned this pull request Oct 25, 2023
Closed
2 tasks
@fatso83

fatso83 commented Oct 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 it does not change the default, as that would be a breaking change, but we can do that with version 18 . It still allows creating sandboxes with log limits, which is probably what you want right now.

EDIT: we changed it to a default of 10k

@fatso83 fatso83 mentioned this pull request Oct 25, 2023
Closed
@fatso83

fatso83 commented Oct 27, 2023

Copy link
Copy Markdown
Contributor

@sgoossens You ok with this? Anything to add/change?

sgoossens
sgoossens commented Oct 27, 2023
View reviewed changes

@sgoossens sgoossens left a comment

Copy link
Copy Markdown
Contributor Author

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

mroderick
mroderick reviewed Oct 27, 2023
View reviewed changes
Comment thread lib/sinon/assert.js Outdated
mroderick
mroderick reviewed Oct 27, 2023
View reviewed changes
Comment thread test/create-sandbox-test.js Outdated
@fatso83
fatso83 merged commit f7d180c into sinonjs:main Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mroderick mroderick mroderick left review comments
@fatso83 fatso83 fatso83 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sgoossens @fatso83 @mroderick