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

feat: Console output test util #1370

Merged
merged 4 commits into from
Jun 12, 2023

Conversation

bmingles
Copy link
Contributor

Console output test util

resolves #1369

@bmingles bmingles requested a review from mofojed June 12, 2023 17:04
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #1370 (c104989) into main (bcce269) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1370   +/-   ##
=======================================
  Coverage   45.55%   45.55%           
=======================================
  Files         506      506           
  Lines       34889    34895    +6     
  Branches     8709     8710    +1     
=======================================
+ Hits        15892    15898    +6     
  Misses      18946    18946           
  Partials       51       51           
Flag Coverage Δ
unit 45.55% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
packages/utils/src/TestUtils.ts 93.54% <100.00%> (+0.69%) ⬆️

@mofojed mofojed requested a review from mattrunyon June 12, 2023 20:00
Comment on lines 55 to 59
/**
* Selectively disable logging methods on `console` object. Uses spyOn so that
* changes will be reverted after leaving the test scope that it is set in. If
* no method names are given, all will be disabled.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would maybe change this to a single param that takes an array and then clarify the intention w/ @param methodNames The console methods to disable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I will add the @param methodNames. Is there a strong reason not to use the rest params though? Looks like it works fine either way.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

The main benefit would be adding params in the future is easier if needed, but I don't expect this to be used too often.

I think part of my tendency also stems from before ...rest existed and you had to use arguments as a magic value if you wanted to use variadic functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll probably just add the jsdoc on this one and leave the rest param unless you feel strongly otherwise

packages/utils/src/TestUtils.ts Outdated Show resolved Hide resolved
packages/utils/src/TestUtils.ts Outdated Show resolved Hide resolved
packages/utils/src/TestUtils.test.tsx Outdated Show resolved Hide resolved
@mattrunyon
Copy link
Collaborator

Do you have a specific example for using this right now? Is it just to disable 3rd party logging like react warning about unmounting component lifecycles? Our log package should be silent during tests by default

@bmingles
Copy link
Contributor Author

There are a few prop type warning / errors in existing tests which might be good candidates. The specific use case though is in setupTests.js I added a console.error to surface hook errors that are otherwise swallowed. For cases where we expect an error, this is a way to disable.

@bmingles bmingles requested a review from mattrunyon June 12, 2023 21:53
@bmingles bmingles merged commit 626de83 into deephaven:main Jun 12, 2023
@bmingles bmingles deleted the 1369-test-util-disable-console branch June 12, 2023 22:17
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.

Test Util - Disable console methods
2 participants