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

Conversion to React Testing Library #1208

Merged
merged 42 commits into from
Dec 9, 2020
Merged

Conversation

merodiro
Copy link
Contributor

@merodiro merodiro commented Dec 4, 2020

Convert tests to React Testing Library as mentioned in #1207

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • documentation is changed or added

@coveralls
Copy link

coveralls commented Dec 4, 2020

Coverage Status

Coverage increased (+0.7%) to 96.372% when pulling ff25ba1 on merodiro:rtl-migration into 3d9c042 on i18next:master.

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 4, 2020

@adrai @jamuhl I just tried converting withSSR.spec.js to RTL, for some reason I get a TypeError, that i18n.off is not a function? Do you have any ideas what could be causing this? Thanks 💕

Test running in watch mode

Screenshot from 2020-12-04 22-04-14

@adrai
Copy link
Member

adrai commented Dec 4, 2020

probably you need to define the off function in the mockI18n object

We discussed about this and it does not seem to be necessary to have screen as a comment nor use it, since we are using toMatchInlineSnapshot for inline snapshots.
@tigerabrodi
Copy link
Contributor

probably you need to define the off function in the mockI18n object

@adrai Works like a charm 🎉 🙌

tigerabrodi and others added 2 commits December 4, 2020 22:29
3 tests.
off method has been added to mockI18n
@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 5, 2020

@adrai @jamuhl In the test/typescript folder, the name of the files have a suffix of .test, I wonder why, since I could not find anything imported from enzyme in those files.

@adrai
Copy link
Member

adrai commented Dec 5, 2020

I don't know, maybe just convention? Maybe @jamuhl knows why?

@tigerabrodi
Copy link
Contributor

I don't know, maybe just convention? Maybe @jamuhl knows why?

@jamuhl I also found many functions in the test/typescript folder, that does not get exported nor used, I wonder if we should keep them? And of course, my question would be, what are those doing?

This would of course be a separate PR if any, sorry for bringing it up in this PR 👍.

Added packages for the Linter setting for TL & Jest-DOM | Cleanup and Refactoring
@tigerabrodi
Copy link
Contributor

Thanks @JacobMGEvans 🥰

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 7, 2020

@JacobMGEvans @balavishnuvj @merodiro @juhanakristian I am fine with this being reviewed by the maintainers 😃.

If you guys are also fine with it, just give this a thumbs-up, otherwise, what do you think still needs to change or be updated?

4 thumbs ups and this should be review-ready I guess 💯

@JacobMGEvans
Copy link
Contributor

@JacobMGEvans @balavishnuvj @merodiro @juhanakristian I am fine with this being reviewed by the maintainers 😃.

If you guys are also fine with it, just give this a thumbs-up, otherwise, what do you think still needs to change or be updated?

4 thumbs ups and this should be review-ready I guess 💯

Do we know why the CircleCI stuff keeps failing?

@tigerabrodi
Copy link
Contributor

@JacobMGEvans No idea tbh, clicked on it and somehow had to login. @adrai @jamuhl Do you guys know why circleci keeps failing? All tests are passing locally 🤷‍♂️

@tigerabrodi
Copy link
Contributor

tigerabrodi commented Dec 7, 2020

CI should not fail anymore after the last commit hopefully 😝 🤞

@adrai
Copy link
Member

adrai commented Dec 7, 2020

I don't see the circleci output eihter, but if I run the tests locally I get an error in "useTranslation.loading.spec.js"
image

@tigerabrodi
Copy link
Contributor

I don't see the circleci output eihter, but if I run the tests locally I get an error in "useTranslation.loading.spec.js"
image

Yeah, this has to be fixed. Currently no ideas, @marcosvega91 do you have an idea? Thanks 🙏 🥰

@marcosvega91
Copy link
Contributor

let me check the code :)

Copy link
Contributor

@marcosvega91 marcosvega91 left a comment

Choose a reason for hiding this comment

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

Let me know if is not clear :)

CI/CD should pass if these assertions work

Co-authored-by: Marco Moretti <[email protected]>
@tigerabrodi
Copy link
Contributor

@adrai and @jamuhl You guys can review this now 😁 🥰

@adrai
Copy link
Member

adrai commented Dec 7, 2020

lgtm, but the boss needs to review it too 😉

@merodiro merodiro requested a review from jamuhl December 8, 2020 10:29
@jamuhl jamuhl merged commit ce174ac into i18next:master Dec 9, 2020
@jamuhl
Copy link
Member

jamuhl commented Dec 9, 2020

was merged

beside that @tigerabrodi have a look at https://github.com/i18next/react-i18next#core-contributors if anyone else feels like planning to help this project in the future - would be happy to add you to the list too!

@jamuhl
Copy link
Member

jamuhl commented Dec 9, 2020

thanks to all helping with this change...having the tests updated to rtl is awesome 👍

@balavishnuvj balavishnuvj deleted the rtl-migration branch December 9, 2020 11:51
@tigerabrodi
Copy link
Contributor

@jamuhl 🥰 💞

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 this pull request may close these issues.

10 participants