Test improvements

[holly-cummins]

• Run node.js tests from maven build
• Add new tests, following example in Rewrite UI in react quarkus-super-heroes#408
• Add null guards so the tests pass (not sure how these tests passed locally before, but I remember running them ...)

I didn't exactly follow the example in quarkusio/quarkus-super-heroes#408 since the React Testing Library folks recommend testing from a user perspective, and using test ids as a last resort. This means the tests need to hardcode more strings and are a bit brittle and coupled, so I'm not totally sold on this as a best practice, but I've adopted it here.

[edeandrea]

React Testing Library folks recommend testing from a user perspective, and using test ids as a last resort.

Is that a true statement? I'm certainly not a UI person, so if that is a better way of doing it I'll be happy to change quarkusio/quarkus-super-heroes#408 around a bit.

[github-actions]

🙈 The PR is closed and the preview is expired.

[holly-cummins]

React Testing Library folks recommend testing from a user perspective, and using test ids as a last resort.

Is that a true statement? I'm certainly not a UI person, so if that is a better way of doing it I'll be happy to change quarkusio/quarkus-super-heroes#408 around a bit.

Well, "true" is true, but "better" is debatable, IMO. :)

https://testing-library.com/docs/queries/about/#priority has a list of priorities for how you should query, with the core principle being "your test should resemble how users interact with your code (component, page, etc.) as much as possible". It has get by text in the middle, and test id is right at the bottom:

getByTestId: The user cannot see (or hear) these, so this is only recommended for cases where you can't match by role or text or it doesn't make sense (e.g. the text is dynamic).

... but it doesn't have any comment on the fact that it makes things super sensitive to trivial little text changes. https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change, from one of the authors, has a fuller explanation of that principle, but again without a commentary on what happens if you change text.

https://stackoverflow.com/questions/69121378/react-testing-library-id-instead-of-data-testid has an interesting comment, which is that if you use id for your test id, then it could also change fairly often as your implementation changes, so it might not be as resilient as I'm saying.

Using regexes (which, in fairness, I have not done!) can reduce the brittleness. I could perhaps have done a bit of getting by role with text as a backup to get the fight button. If you're going to copy these back, I'll do a light rework to make them a bit more resilient where I can.

[edeandrea]

When I committed back to the sample app yesterday I actually changed things around to use roles after reading your initial comment here...

  it("renders a table with column headings", async () => {
    await act(async () => {
      render(<App/>)
    })

    const table = screen.getByRole("table")
    expect(table).toBeInTheDocument()

    const thead = within(table).getAllByRole('rowgroup')[0]
    const headRows = within(thead).getAllByRole("row")
    const headCols = within(headRows[0]).getAllByRole("columnheader")

    expect(headCols).toHaveLength(5)
    expect(headCols[0]).toHaveTextContent("Id")
    expect(headCols[1]).toHaveTextContent("Fight Date")
    expect(headCols[2]).toHaveTextContent("Winner")
    expect(headCols[3]).toHaveTextContent("Loser")
    expect(headCols[4]).toHaveTextContent("Location")

    const tbody = within(table).getAllByRole('rowgroup')[1];
    const bodyRows = within(tbody).getAllByRole('row');
    const rowCols = within(bodyRows[0]).getAllByRole("cell")

    expect(rowCols).toHaveLength(5)
    expect(rowCols[0]).toHaveTextContent(fight.id)
    expect(rowCols[1]).toHaveTextContent(fight.fightDate)
    expect(rowCols[2]).toHaveTextContent(fight.winnerName)
    expect(rowCols[3]).toHaveTextContent(fight.loserName)
    expect(rowCols[4]).toHaveTextContent(fight.location.name)
  })

[holly-cummins]

I like the use of roles, I think I don't love the very detailed assertions about the row headings. It feels a bit like it's getting into 'writing the implementation down twice' territory. I can't think of many scenarios where the header text would change and it wouldn't have been the result of an intentional change. For example, we might adjust a spelling, or swap row order.

But sharing code is useful, and it's not a big deal, so I will take them as is. Thanks for sharing back :)

[edeandrea]

I think I don't love the very detailed assertions about the row headings. It feels a bit like it's getting into 'writing the implementation down twice' territory.

I actually thought about that as I was doing it but then I decided to go ahead with it for a couple of reasons:

1. What if the headers were dynamic/loaded from somewhere? They currently aren't, so this could be classified as "premature optimization" I guess :)
2. Making sure there aren't typos somewhere?

I could be convinced to do it or not to do it.

[holly-cummins]

Yeah, I reckon reason #1 is premature optimisation, and unit tests are a pretty heavy tool for finding one-off typos. :)

On the other hand, the thing that's likely to make this test fail is re-ordering the columns, and if we re-order the columns, the second set of assertions about what's in column 1, 2, 3, etc would need to be updated anyway. So now that we have the tests, I don't think there's much to be gained by deleting them.

[holly-cummins]

Actually, I have another objection, which I think is more important :)

I think this level of test should be in the FightList.test.js, not the App.test.js. It's nothing to do with the integration that the App component does, it's just about fairly low-level details of how the FightList component lays out content. So I'm moving it in my copy to FightList test.

That's one nice thing about React, it's easier to isolate things (and tests) to smaller components.

[edeandrea]

I think I actually have similar tests in FightList.test also. I wasn't sure what else to test in App.test, since it's the parent for fight and fight list.

[holly-cummins]

I think I actually have similar tests in FightList.test also. I wasn't sure what else to test in App.test, since it's the parent for fight and fight list.

I think it possibly doesn't need much - or at least, while it's not doing much, it doesn't need much. IMO, you need to make sure there's evidence of it pulling in the child components, but not details of the child components. Those are covered elsewhere and should be encapsulated away from the parent tests. If the parent starts doing more (like with #436), then of course the tests need to be more extensive.

I find in React even having a dead simple test for a component that checks it renders without error catches a huge number of issues.

[edeandrea]

Or maybe testing that interactions between components work as expected? :)

[holly-cummins]

Or maybe testing that interactions between components work as expected? :)

Well, yes. :)

At the time of writing, there were no interactions between components, so there was nothing to test :) ... As part of #436 I've added the interactions, so since App.js is smarter, its tests are doing more. (But it still shouldn't care about things like the order of columns in the fight table.)

[edeandrea]

(But it still shouldn't care about things like the order of columns in the fight table.)

You've convinced me and i've removed that in the sample too.