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.

Sign up for GitHub

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

Jump to bottom

chore(payments): clean Up Test Output #13182

Merged
merged 1 commit into from
Jun 8, 2022
Merged

chore(payments): clean Up Test Output #13182

merged 1 commit into from
Jun 8, 2022

Conversation

IvoJP
Copy link
Contributor

@IvoJP IvoJP commented Jun 7, 2022

Because:

  • The payments test output is overly verbose for the logger tests and also contains some warnings that need to be addressed

This commit:

  • overrides the console for logger tests, cleans up some react tests so that all interactions/side effects are accounted for, updates how the page title is displayed (its lifecycle events are deprecate and unavailable in react 17), and adds a check for if the cancelSubscriptionPanel component is mounted before a specific state update is applied (this is a memory leak apparently)

Closes #13011

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

@IvoJP IvoJP force-pushed the fxa-5161/cleanup-payments-tests branch 3 times, most recently from ebbf8e7 to 7d7ba6b Compare June 7, 2022 16:54
@IvoJP IvoJP requested a review from StaberindeZA June 7, 2022 17:50
@IvoJP IvoJP marked this pull request as ready for review June 7, 2022 17:50
@IvoJP IvoJP requested a review from a team as a code owner June 7, 2022 17:50
StaberindeZA
StaberindeZA requested changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

Ah so nice! The test output looks so much better!

I just have 2 change requests. It'd be great if we could reuse the <Head> component. Regarding the act it'd be nice to not have to use it, but it shouldn't block the PR if it's not an easy fix.

I did notice that there are still some console.logs showing up in the server tests. If it's a simple fix, could you fix these as well? (See screenshot below)

image

packages/fxa-payments-server/src/App.tsx Outdated Show resolved Hide resolved
packages/fxa-payments-server/src/components/CouponForm/index.test.tsx Outdated Show resolved Hide resolved
@IvoJP IvoJP force-pushed the fxa-5161/cleanup-payments-tests branch from 9cd6fce to 2281f66 Compare June 8, 2022 07:32
@IvoJP
Copy link
Contributor Author

IvoJP commented Jun 8, 2022 •
edited
Loading

I was not able to block the output in your screenshot, at least my few hours of attempts were unfruitful. What makes those different from blocking similar output in this PR is that those logging statements use a different logger (mozlog) and the calls to the logger are not coming from the tests, they are coming from the server.js and route-logging.js files. For a few test files the server has to be instantiated with the call const server = require('./server')() I was not able to either mock or intercept the calls to log stuff easily with how this is setup; these even seemed to circumvent hijacking the console object at the jest config level (i.e. setting a differnt global console for the context jest is running in). I started to get pretty invasive but not only was I not successful but it started to get kind of hacky especially considering the minor gains in the test ouput.

@IvoJP
chore(payments): clean Up Test Output
752f09c 
Because:

* The payments test output is overly verbose for the logger tests and also contains some warnings that need to be addressed

This commit:

* overrides the console for logger tests, cleans up some react tests so that all interactions/side effects are accounted for, updates how the page title is displayed (its lifecycle
events are deprecate and unavailable in react 17), and adds a check for if the cancelSubscriptionPanel component is mounted before a specific state update is applied (this is a memory
leak apparently)

Closes #13011
@IvoJP IvoJP force-pushed the fxa-5161/cleanup-payments-tests branch from 2281f66 to 752f09c Compare June 8, 2022 08:23
@StaberindeZA StaberindeZA self-requested a review June 8, 2022 14:00
StaberindeZA
StaberindeZA approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the changes.

@IvoJP IvoJP merged commit 0023c32 into main Jun 8, 2022
@IvoJP IvoJP deleted the fxa-5161/cleanup-payments-tests branch June 8, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StaberindeZA StaberindeZA StaberindeZA approved these changes

Assignees

@StaberindeZA StaberindeZA

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[subplat] Cleanup fxa-payments-server test output
2 participants
@IvoJP @StaberindeZA