Incremental UI Tests #4 #83

aradmargalit · 2021-01-04T06:51:28Z

Review #82 first! Then rebase this and merge.

aradmargalit · 2021-01-04T06:52:01Z

client/src/components/FrequencyChart/FrequencyChart.test.tsx

+
+    it('renders Infinity per Week', () => {
+      render(<FrequencyChart reviews={reviews} />);
+      expect(screen.getAllByText('Infinity')).toBeDefined();


I expect this will change once glue gets merged in -- happy to update once that's ready, but this test will be a good canary in that coalmine.

aradmargalit · 2021-01-04T06:52:35Z

client/src/components/MenuBar/MenuBar.test.tsx

+      expect(screen.getByText(displayName)).toBeDefined();
+    });
+
+    // TODO EM: I think this should be a <Link /> so that it's easier to test.


I'm getting weird errors about how href based navigation isn't supported in JSDOM

I have no complaints about moving all href -> Link

Should I commit that on this branch before merging?

Nah, I made an issue for it
#84

aradmargalit · 2021-01-04T06:52:57Z

client/src/components/MinimalStatBox/MinimalStatBox-view.tsx

@@ -20,7 +20,7 @@ export default function MinimalStatBoxView({ userDisplayName, reviews }: Minimal
  );

  // destructure if not null, or set all to undefined
-  const { numReviews, ppwString, ppwColor } = getReviewStats(reviews) || {};
+  const { numReviews, ppwString, ppwColor } = getReviewStats(reviews);


I couldn't find any way that the || {} would ever trigger. Can you?

Hmm no, I think what you have is safe.
The reviews arg is typed to be Review[] so TS compiler would've complained if there were a valid route to passing undefined or null instead, right?

That was my thinking as well, yeah

aradmargalit · 2021-01-04T06:53:45Z

client/src/components/PaperSearchBar/PaperSearchBar-view.tsx

@@ -135,7 +135,7 @@ export default function PaperSearchBarView({
    <div>
      <br />
      {searchArea}
-      {loading ? <Spin className="searchResult loading-spinner" /> : searchRender}
+      {loading ? <Spin data-testid="paper-searchbar-spinner" className="searchResult loading-spinner" /> : searchRender}


using data-testid should really be a last resort since it's an accessibility code smell. But in this case, it's antds component, so I'm just tagging it so I can check for it in tests.

eshedmargalit

This PR moved me from "tests are nice but mostly just an extra safety step to catch corner cases" to "wow, I can actually program my entire manual check workflow into an automated test suite, it's lit 🔥 "

Thanks so much for your work on this and #81, #82 !

eshedmargalit · 2021-01-10T15:16:51Z

client/src/components/FrequencyChart/FrequencyChart.test.tsx

+    it('renders about 1 per week', () => {
+      render(<FrequencyChart reviews={reviews} />);
+      expect(screen.getAllByText('1')).toBeDefined();
+    });


Question: it seems like the test criteria in this test and the preceding test are pretty vague, i.e.,

screen.getAllByText('1')) doesn't really test that ppw = 1

If ppw = 4.01, for example, both tests pass (but shouldn't)

Thoughts?

Yeah, good point. It's partially because react testing library can't search for text across HTML elements, which makes searching for the full string hard. Let's open an issue to clean these up later.

eshedmargalit · 2021-01-10T15:20:55Z

client/src/components/Login/Login.test.tsx

+
+  describe('with a logged-in user', () => {
+    it('redirects to the dashboard', () => {
+      const initialState = { ...getBlankInitialState(), user: { ...blankUser, display: 'Piranesi' } };


Hmm. This passes because technically a User is present and it does redirect. But display isn't a valid attribute of User and it wouldn't show their name correctly (should be displayName).

I guess this test doesn't actually test that the displayName works as expected, so maybe okay?

ah that was just a typo, let me push a fix before we merge

eshedmargalit · 2021-01-10T15:21:19Z

client/src/components/MenuBar/MenuBar.test.tsx

+    it('shows the information icon with information on hover', async () => {
+      renderWithRouterRedux(<MenuBarRedux />);
+
+      userEvent.hover(screen.getByLabelText('info-circle'));


eshedmargalit · 2021-01-10T15:21:57Z

client/src/components/MenuBar/MenuBar.test.tsx

+      expect(screen.getByText(displayName)).toBeDefined();
+    });
+
+    // TODO EM: I think this should be a <Link /> so that it's easier to test.


I have no complaints about moving all href -> Link

Should I commit that on this branch before merging?

eshedmargalit · 2021-01-10T15:23:39Z

client/src/components/MinimalStatBox/MinimalStatBox-view.tsx

@@ -20,7 +20,7 @@ export default function MinimalStatBoxView({ userDisplayName, reviews }: Minimal
  );

  // destructure if not null, or set all to undefined
-  const { numReviews, ppwString, ppwColor } = getReviewStats(reviews) || {};
+  const { numReviews, ppwString, ppwColor } = getReviewStats(reviews);


Hmm no, I think what you have is safe.
The reviews arg is typed to be Review[] so TS compiler would've complained if there were a valid route to passing undefined or null instead, right?

eshedmargalit · 2021-01-10T15:24:00Z

client/src/components/NotFound/NotFound.test.tsx

+import NotFound from './NotFound';
+
+describe('<NotFound />', () => {
+  it('shows a stupid compass lol?', () => {


This is cyber-bullying

Let's hire Re'em to make a 404 page

Jokes aside, I think that'd be a really nice gesture

eshedmargalit · 2021-01-10T15:25:36Z

client/src/components/PaperSearchBar/PaperSearchBar.test.tsx

+
+    it('the second one redirects to the form page', () => {
+      renderWithRouterRedux(<PaperSearchBar />, { redirectTo: '/form' });
+      userEvent.click(screen.getAllByText(/Create Manual Entry/)[1]);


Okay nice, I was wondering how this would get resolved 👀

* shhhh is okay * self review feedback * Basic FrequencyChart tests * Login tests * MenuBar tests * test todos * Remove impossible conclusion * safe assertion, I hope * NotFound test * bare bones paper search * more paper search tests * whoo, that should be it! * display --> displayName

aradmargalit commented Jan 4, 2021

View reviewed changes

Base automatically changed from inc-ui-test-3 to react-ts January 10, 2021 02:45

aradmargalit added 12 commits January 10, 2021 10:03

shhhh is okay

4dfed9a

self review feedback

a39dc10

Basic FrequencyChart tests

7bee04e

Login tests

72ec17a

MenuBar tests

3d5afd8

test todos

dc1f296

Remove impossible conclusion

24a1b74

safe assertion, I hope

04d50ff

NotFound test

c646aa0

bare bones paper search

0a253c3

more paper search tests

a60749b

whoo, that should be it!

baa811c

eshedmargalit force-pushed the inc-ui-test-4 branch from a7d2e5f to baa811c Compare January 10, 2021 15:07

eshedmargalit reviewed Jan 10, 2021

View reviewed changes

aradmargalit mentioned this pull request Jan 10, 2021

I have no complaints about moving all href -> Link #84

Closed

display --> displayName

cbe10cd

eshedmargalit merged commit b212687 into react-ts Jan 10, 2021

eshedmargalit deleted the inc-ui-test-4 branch January 10, 2021 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incremental UI Tests #4 #83

Incremental UI Tests #4 #83

aradmargalit commented Jan 4, 2021

aradmargalit Jan 4, 2021

aradmargalit Jan 4, 2021

eshedmargalit Jan 10, 2021

aradmargalit Jan 10, 2021

aradmargalit Jan 4, 2021

eshedmargalit Jan 10, 2021

aradmargalit Jan 10, 2021

aradmargalit Jan 4, 2021

eshedmargalit left a comment

eshedmargalit Jan 10, 2021

aradmargalit Jan 10, 2021

eshedmargalit Jan 10, 2021

aradmargalit Jan 10, 2021

eshedmargalit Jan 10, 2021

eshedmargalit Jan 10, 2021

eshedmargalit Jan 10, 2021

eshedmargalit Jan 10, 2021

aradmargalit Jan 10, 2021

eshedmargalit Jan 10, 2021

Incremental UI Tests #4 #83

Incremental UI Tests #4 #83

Conversation

aradmargalit commented Jan 4, 2021

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

eshedmargalit 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