Add a basic framework for testing and a single demo test #133

aswinmprabhu · 2019-03-20T18:14:48Z

alexdor

Hi, thanks for the pr : ) I just have a few questions

alexdor · 2019-03-21T08:07:48Z

package.json

@@ -96,7 +104,7 @@
    "start-js": "react-scripts-ts start",
    "start": "npm-run-all -p watch-css start-js",
    "build": "yarn build-css && react-scripts-ts build && ncp build dist",
-    "test": "react-scripts-ts test --env=jsdom",


Why did you replace this with the jest command? Doesn't react-scripts-ts use jest for testing?

There seems to be some problem with the jest config of react-scripts-ts (jestjs/jest#6393). It doesn't really matter since both commands just run jest in watch mode.

Not exactly, react-scripts-ts runs ts-jest not jest

I am using ts-jest as a preset in the jest config

alexdor · 2019-03-21T08:08:23Z

package.json

@@ -64,11 +64,19 @@
    "utility-types": "^3.4.1"
  },
  "devDependencies": {
+    "@types/enzyme": "^3.9.1",
+    "@types/enzyme-adapter-react-16": "^1.0.5",
+    "enzyme": "^3.9.0",


Why do we need enzyme? Doesn't react-testing-library cover our use case?

I would also have preferred to use react-testing-library. But I had trouble getting it to work with typescript. Its documentation and examples are not as good or mature as enzyme.

I think jest along with react-testing-library serves the purpose

I like the approach that react-testing library takes to testing. But like I said, it is not as well documented or mature as enzyme is

alexdor · 2019-03-21T08:09:10Z

src/App.test.tsx

@@ -1,9 +0,0 @@
-import React from "react";


Why did you drop this test?

This test will have to be replaced with an enzyme based one. But that requires some more work for getting the router and store to work in a testing environment. This PR is meant to just establish a base framework.

react-testing-library is an official replacement for enzyme. There's a big blog written by kent c dodds why he discourages enzyme and it's shallow render feature

What do you mean by "official"? React doesn't have an official testing suite. Enzyme is still there along with others in the official react docs. If you can point me to the react-testing-library docs for typescript, I am more than happy to try it out.
ReferenceError: document is not defined is the error I am getting when I try const { getByTestId } = render(<FixmeNavbar />);. A quick google for the error doesn't show any helpful results. Any idea why this happens?

What do you mean by "official"? React doesn't have an official testing suite. Enzyme is still there along with others in the official react docs. If you can point me to the react-testing-library docs for typescript, I am more than happy to try it out.

Why do you have to write your tests in that? I guess its not mandatory for writing your tests in typescript

I'll push this configuration as a PR and will also share my fork with you in a moment

@aswinmprabhu here:-
https://github.com/nik72619c/fixme/tree/nik

When I replace Spinner with App, this is the error that I get. This one of the errors I got when I was setting up react-testing-library. Then I followed the official react-testing-library docs regarding react-router but that didn't work either. A similar error occurs when I try to render FixMeNavbar as well.

@aswinmprabhu can you check if this is still happening at #134 . Though if the test is a snapshot test then switching the component should break the test. You can read more about snapshots here https://jestjs.io/docs/en/snapshot-testing

@aswinmprabhu Could you add this one back? It isn't a huge test but it's important to test that the app doesn't crash

siddhant1 · 2019-03-22T05:57:35Z

I don't think we are using css modules so I don't feel the need of fileMock

aswinmprabhu · 2019-03-22T06:31:36Z

fileMock is for svg images not css modules.

…

On Fri, Mar 22, 2019, 11:27 AM siddhant ***@***.***> wrote: I don't think we are using css modules so I don't feel the need of fileMock — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#133 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeGKdn7KKzS-ZsWbBgbBMHpB8HdR11U-ks5vZHDQgaJpZM4b_9Jv> .

siddhant1 · 2019-03-23T13:45:21Z

I am personally not in favour of react-testing-library , I think we should use enzyme.

…

On Sat, 23 Mar, 2019, 7:13 PM Nikhil Sharma, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/App.test.tsx <#133 (comment)>: > @@ -1,9 +0,0 @@ -import React from "react"; I'll push this configuration as a PR and will also share my fork with you in a moment — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#133 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AdJoBggO38nm49lOMICwdh3IPNwVVj6Qks5vZi9zgaJpZM4b_9Jv> .

nik72619c · 2019-03-23T13:57:29Z

I am personally not in favour of react-testing-library , I think we should use enzyme.
…
On Sat, 23 Mar, 2019, 7:13 PM Nikhil Sharma, @.> wrote: @.* commented on this pull request. ------------------------------ In src/App.test.tsx <#133 (comment)>: > @@ -1,9 +0,0 @@ -import React from "react"; I'll push this configuration as a PR and will also share my fork with you in a moment — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#133 (comment)>, or mute the thread <github.com/notifications/unsubscribe-auth/AdJoBggO38nm49lOMICwdh3IPNwVVj6Qks5vZi9zgaJpZM4b_9Jv> .

Enzyme is good but it poses problems in the long run.

siddhant1 · 2019-03-23T13:59:59Z

I don't see any problem with companies working with enzyme , Airbnb uses it and it's not an issue for them.

alexdor · 2019-03-25T08:36:04Z

Even though enzyme is used by Airbnb and others it doesn't mean that it's always the correct solution. Enzyme is a huge project and a lot of times it's hurtful for the codebase. The tests shouldn't become coupled with the implementation of the component (which a lot of times is happening with enzyme) the tests should test the component as a user interacting with the component so they can be reused if the implementation changes. I would much prefer to use react-testing-library because it's a simpler project and it aligns more with the philosophy of "don't couple your tests with the implementation of the component".

aswinmprabhu · 2019-04-17T06:17:58Z

I managed to get react-testing-library working with typescript. I added back the test for App component as well after setting it up with react-router.

aswinmprabhu added 2 commits March 20, 2019 22:52

feat: Add a testing framework

0f914e5

feat: Modify the yarn test script

95cccf9

alexdor reviewed Mar 21, 2019

View reviewed changes

fix: Remove react-testing-library from deps

26a9a2f

aswinmprabhu added 4 commits April 17, 2019 11:22

feat: Setup react-testing-library and migrate App.test.tsx

648896d

chore: Remove enzyme and fix jest config

9f32d07

chore: Remove the NavBar test

fef0131

chore: Add app snapshot

bdb5e60

fix: Test container instead of the whole component

c84051a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a basic framework for testing and a single demo test #133

Add a basic framework for testing and a single demo test #133

aswinmprabhu commented Mar 20, 2019

alexdor left a comment

alexdor Mar 21, 2019

aswinmprabhu Mar 21, 2019

alexdor Mar 22, 2019

aswinmprabhu Mar 22, 2019

alexdor Mar 21, 2019

aswinmprabhu Mar 21, 2019

nik72619c Mar 23, 2019

aswinmprabhu Mar 23, 2019

alexdor Mar 21, 2019

aswinmprabhu Mar 21, 2019

nik72619c Mar 23, 2019

aswinmprabhu Mar 23, 2019 •

edited

Loading

nik72619c Mar 23, 2019

nik72619c Mar 23, 2019

nik72619c Mar 23, 2019

aswinmprabhu Mar 24, 2019

alexdor Mar 25, 2019

alexdor Apr 5, 2019

siddhant1 commented Mar 22, 2019

aswinmprabhu commented Mar 22, 2019 via email

siddhant1 commented Mar 23, 2019 via email

nik72619c commented Mar 23, 2019

siddhant1 commented Mar 23, 2019

alexdor commented Mar 25, 2019

aswinmprabhu commented Apr 17, 2019

Add a basic framework for testing and a single demo test #133

Are you sure you want to change the base?

Add a basic framework for testing and a single demo test #133

Conversation

aswinmprabhu commented Mar 20, 2019

alexdor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aswinmprabhu Mar 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddhant1 commented Mar 22, 2019

aswinmprabhu commented Mar 22, 2019 via email

siddhant1 commented Mar 23, 2019 via email

nik72619c commented Mar 23, 2019

siddhant1 commented Mar 23, 2019

alexdor commented Mar 25, 2019

aswinmprabhu commented Apr 17, 2019

aswinmprabhu Mar 23, 2019 •

edited

Loading