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(logger): disable logs while testing with jest --silent in dev env #1165

Conversation

shdq
Copy link
Contributor

@shdq shdq commented Nov 14, 2022

Description of your changes

It conditionally initializes console property as an instance of the internal version of Console() class (PR #748) or as the global node console if the `POWERTOOLS_DEV' env variable is set and has a truthy value.

How to verify this change

Related issues, RFCs

Issue number:
#1030

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
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I 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.

@pull-request-size pull-request-size bot added the size/M PR between 30-99 LOC label Nov 14, 2022
@github-actions github-actions bot added feature logger This item relates to the Logger Utility labels Nov 14, 2022
@dreamorosi
Copy link
Contributor

Also I think it could be interesting to surface this new behavior in the docs, specifically in this section.

We could add a new sub section under Testing your code

@dreamorosi dreamorosi linked an issue Nov 14, 2022 that may be closed by this pull request
docs/core/logger.md Outdated Show resolved Hide resolved
@shdq
Copy link
Contributor Author

shdq commented Nov 14, 2022

What about testing? Is there a way to unit test this "silent" behavior? I only came up with the idea to somehow check what type of console object we use with POWERTOOL_DEV on and off.

Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
@dreamorosi
Copy link
Contributor

What about testing? Is there a way to unit test this "silent" behavior? I only came up with the idea to somehow check what type of console object we use with POWERTOOL_DEV on and off.

I don't think we should test Jest's behavior. From my point of view we just need to test that when POWERTOOL_DEV is enabled or disabled the value of Logger.console is a new Console object or the global one.

@dreamorosi dreamorosi added feature PRs that introduce new features or minor changes and removed feature labels Nov 14, 2022
@pull-request-size pull-request-size bot added size/L PRs between 100-499 LOC and removed size/M PR between 30-99 LOC labels Nov 14, 2022
@shdq
Copy link
Contributor Author

shdq commented Nov 14, 2022

From my point of view we just need to test that when POWERTOOL_DEV is enabled or disabled the value of Logger.console is a new Console object or the global one.

This is what I meant actually.

I tested the global node object when POWERTOOLS_DEV is set. But for the other option, since instances of a class are not the same I presumed the opposite:

expect(logger).not.toEqual({
  ...logger,
  console: console,
});

@dreamorosi what do you think?

@dreamorosi
Copy link
Contributor

I think that's acceptable, maybe just leave a comment right above the assertion for future reference to explain the assertion.

@dreamorosi dreamorosi self-requested a review November 14, 2022 14:32
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Sorry, I wasn't 100% clear with my previous comment.

In the changes related to the docs, could we please add a code snippet that shows how to call jest with the flag & with the environment variable, similar to what done here?

i.e.

export POWERTOOLS_DEV=true && npx jest --silent

@shdq
Copy link
Contributor Author

shdq commented Nov 14, 2022

Sorry, I wasn't 100% clear with my previous comment.

I missed the comment it happens in remote communication. I'm pleased to finish the job appropriately. Let me know if some other changes are needed.

@dreamorosi dreamorosi self-requested a review November 14, 2022 17:09
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thank you @shdq!

Tomorrow morning I will test the code locally (same process as the one in the issue) & see how it renders in the documentation - then if it's all good I'll approve & we can merge :)

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Hi @shdq, I have reviewed the code and it looks good. I also have tested this version in a separate codebase & verified that the `--silent- flag is indeed respected.

I'm going to run the integration tests on this branch now.

In the meanwhile I have left a comment/question about one of the sections of your changes.

Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
@dreamorosi dreamorosi self-requested a review November 15, 2022 10:40
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Integration tests are passing: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3469565442

@shdq thank you so much for all the hard work here!

@dreamorosi dreamorosi added enhancement PRs that introduce minor changes, usually to existing features and removed feature PRs that introduce new features or minor changes labels Nov 15, 2022
@dreamorosi dreamorosi merged commit 6f0c307 into aws-powertools:main Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that introduce minor changes, usually to existing features logger This item relates to the Logger Utility size/L PRs between 100-499 LOC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: disable logs while testing with jest
3 participants