Skip to content

Unit test for fluent-react withLocalization fails inconsistently #379

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

Closed
flut1 opened this issue Jun 20, 2019 · 2 comments · Fixed by #384
Closed

Unit test for fluent-react withLocalization fails inconsistently #379

flut1 opened this issue Jun 20, 2019 · 2 comments · Fixed by #384

Comments

@flut1
Copy link
Contributor

flut1 commented Jun 20, 2019

I noticed that my pull request #378 was failing in Travis on the following test:

getString with access to the l10n context, with message changes

Upon further investigation, it seems like this also fails on the master branch, but not consistently. If I run all tests for fluent-react, they seem to sometimes succeed, and if I run this test in isolation it seems to always succeed.

I can think of 2 possible reasons:

  • The tests are somehow affecting each other. The test is using the full rendering API from Enzyme. Their docs say that tests may affect each other.
  • The test contains a race condition. The line l10n.setBundles([newBundle]); causes a force-update in WithLocalization, but maybe React doesn't flush these changes synchronously.
@flut1
Copy link
Contributor Author

flut1 commented Jun 20, 2019

My suggestion would be to add wrapper.update() before the assertion here:

This seems to fix it. Is it ok to force-update the component or should the test also assert that WithLocalization updates by itself?

A similar test in localized_change_test.js seems to already have this wrapper.update() statement before the last assertion:

@stasm
Copy link
Contributor

stasm commented Jul 1, 2019

Thanks for the investigation, @flut1 :) I opened #384 to add wrapper.update() as you suggest. This should be sufficient for the time being. As you note, wrapper.update() is already used in other tests to serve the similar purpose.

The tests will need to be reviewed anyways when I start working on #182, which I hope is soon.

@stasm stasm closed this as completed in #384 Jul 1, 2019
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 a pull request may close this issue.

2 participants