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

test(logger): added unit test for URIError #451

Merged

Conversation

JavierMendozaGomez
Copy link
Contributor

@JavierMendozaGomez JavierMendozaGomez commented Jan 10, 2022

Description of your changes

Added unit test for the URIError type to increase the coverage of the log error formatter.

How to verify this change

Run
npm test packages/logger/tests/unit/formatter/PowertoolLogFormatter.test.ts

Related issues, RFCs

No RFC's were created as this is not a significant work.

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

package-lock.json Outdated Show resolved Hide resolved
@dreamorosi dreamorosi added the logger This item relates to the Logger Utility label Jan 10, 2022
@dreamorosi
Copy link
Contributor

Hello @JavierMendozaGomez thanks for opening the PR.

Just to make sure we are all on the same page I have a couple of questions:

  • Was the coverage below 100%?
  • Could you elaborate a bit on what kind of case this test is trying to cover?
  • Why this type of error deserves special treatment?

Thank you in advance!

@JavierMendozaGomez
Copy link
Contributor Author

JavierMendozaGomez commented Jan 12, 2022

Hello @dreamorosi . Sorry for my confusing description of the PR.
I was going through your unit test and I saw that you were making unit test to check the format of the error for each one of the possible errors. You had the ReferenceError, AssertionError, RangeError, SyntaxError, TypeError and I realised that you were missing the URIError on your tests.
The jest coverage was at 100 per cent before the addition. What I mean was increase in the coverage of the format of the log of the errors adding the URIError.

@dreamorosi dreamorosi requested a review from alex-m-aws January 17, 2022 14:22
@dreamorosi dreamorosi added this to the production-ready-release milestone Jan 17, 2022
@flochaz flochaz merged commit 5a28b32 into aws-powertools:main Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logger This item relates to the Logger Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants