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

Fix/solve snapshots tests failing node 18 #15436

Merged
merged 8 commits into from Jan 16, 2023

Conversation

simotae14
Copy link
Contributor

@simotae14 simotae14 commented Jan 13, 2023

What does it do?

It solves the problem that we have with node 18.13 running the snapshots test, the problem was the unicode space character

Why is it needed?

Because we have the job related to the frontend unit tests in node 18 failing

How to test it?

You can try to run locally the unit test in node 18.13

@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Base: 60.48% // Head: 60.48% // No change to project coverage 👍

Coverage data is based on head (32892c2) compared to base (5e1fda7).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15436   +/-   ##
=======================================
  Coverage   60.48%   60.48%           
=======================================
  Files        1353     1353           
  Lines       33202    33202           
  Branches     6353     6353           
=======================================
  Hits        20081    20081           
  Misses      11280    11280           
  Partials     1841     1841           
Flag Coverage Δ
back 50.37% <ø> (ø)
front 65.03% <ø> (ø)
unit_back 50.37% <ø> (ø)
unit_front 65.03% <ø> (ø)

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@simotae14 simotae14 self-assigned this Jan 13, 2023
@simotae14 simotae14 added source: dependencies Source is dependency problem pr: fix This PR is fixing a bug labels Jan 13, 2023
@simotae14 simotae14 added this to the 4.6.0 milestone Jan 13, 2023
innerdvations
innerdvations previously approved these changes Jan 16, 2023
const reactIntl = jest.requireActual('react-intl');
const intl = reactIntl.createIntl({
locale: 'en',
messages: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you store these messages as a constant above please and then re-use them in the two cases they are used?

joshuaellis
joshuaellis previously approved these changes Jan 16, 2023
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Worth writing a bit of documentation in /docs under testing/unit about this? Otherwise people will question why we're mocking react-intl :)

Could be a second PR to get this one merged though.

@gu-stav
Copy link
Contributor

gu-stav commented Jan 16, 2023

@joshuaellis I think I'd prefer to have a FE (sync) discussion about this first. To avoid developers running into this we should find a more global solution to this IMO (e.g. by globally mocking react-intl or Date). However this is a good emergency solution for now to fix the tests.

What do you think?

@joshuaellis
Copy link
Member

I think at minimum a code comment should be there with those mocks – plans change, priorities are edited, who's to say when the permanent solution will be found and implemented? And I'd rather have that safety net then loose it!

@simotae14 simotae14 merged commit 33a05a9 into main Jan 16, 2023
@simotae14 simotae14 deleted the fix/solve-snapshots-tests-failing-node-18 branch January 16, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: dependencies Source is dependency problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants