Behavioural unit tests for navigation and reference #5085

Iku-turso · 2022-03-23T16:21:21Z

As motivation, behavioural unit tests:

Test orchestration of main, renderer, etc., ie. the whole application, as if done by a user.
Work also as documentation. This documentation is always up to date by virtue of tests being green.
Cover a lot of production code with reasonably few lines of test code.
Are fast, and thus support more efficient development (eg. TDD).

See conversation for more info.

Not to be merged before #5084.

jansav · 2022-03-28T05:36:37Z

Note: Updated base to make diff easier to read.

panuhorsmalahti · 2022-03-28T12:46:58Z

src/behaviours/cluster/__snapshots__/order-of-sidebar-items.test.tsx.snap

@@ -0,0 +1,719 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


I think large snapshots are not a good thing. It makes it difficult to review and understand diffs, which can hide regressions, and in particular exposes rather technical details like classnames to the diff. I think snapshots, if used, should be kept as small as semantically reasonable. Alternatively, the snapshot assertion could be replaced with a manully defined assertion (about the order, for example for this test).

These snapshots are real lifesavers some times (e.g. unexpected change in snapshot). Snapshots are used only as additions and functionality should be tested separately. That being said, it's nice to know what are the changes in DOM.

I think large snapshots have more costs than benefits and I wouldn't approve this PR with these.

I also don't like that snapshot based testing locks in the DOM, I think that the user doesn't care at all about it and more cares about the text then if an extra <div> is added somewhere.

Again: Reasoning for snapshots is different. Snapshots are very important for detecting unexpected changes in UI caused by some change in somewhere else.

Please, provide more than you opinions. Concreate examples how you would solve the issue described here and in slack. Snapshots provide easy solution without minimal downsides (if any).

I don't agree that this should be a requirement for testing. We want tests for UI components to test from a "user perspective", not asserting a specific DOM structure. If we have a good test coverage of unit tests of UI components we can be confident that the UI components work correctly, making snapshots unnecessary. I think that it's a rare edge case where these snapshots would be actionable for developers, thus making the downsides exceed the benefits.

The downsides of large snapshots are
a) difficult to review snapshot diffs, making them mostly useless
b) creating noise to diffs
c) having to update the snapshots when DOM changes
d) creating an incentive for developers to rely on a snapshot for asserting a semantic meaning of the DOM tree

If we have a good test coverage of unit tests

We don't have. And we won't have in near future. And we won't have ever, as long as whole team doesn't write code in TDD.

--
a) Large snapshot diffs are not supposed to be reviewed. That doesn't mean they are useless.
b) Read a)

c) It's not downside that you get instant feedback from unit tests that something has changed. Current situation in code base is that components are re-used in so many places that unexpected places need to be caught. And this doesn't affect only components, even though components are the ones that are rendered. How many bugs we have seen caused by some change in somewhere else? I can answer that: too many. If there is a way to give instant feedback to developer, we should definitely use it.

d) This is where PR review comes to place. No functionality should be tested with snapshots.

Yes, the goal is to have good coverage.

The downside is having to do a manual step to update the snapshots (even if that step is pretty small).

github-actions · 2022-03-28T12:52:13Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2022-03-28T13:11:43Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

panuhorsmalahti · 2022-04-01T12:51:42Z

src/behaviours/navigate-to-extension-page.test.tsx

+
+    describe("when changing page parameters", () => {
+      beforeEach(() => {
+        const button = rendered.getByTestId("button-to-change-page-parameters");


I think getByTestId should be avoided and getByText etc. should be preferred.

See best practice in the official docs: https://testing-library.com/docs/queries/bytestid/

Read the post linked in the page you provided. https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change

I read it, and it agrees with my review comment..

To be more specific: using test ids is okay if other queries are not reliable / possible.

Quote from the post:
"Sometimes you can't reliably select an element by any of the other queries. For those, it's recommended to use data-testid"

Longer quote from the post:
"This is why Testing Library has the queries that it does. The queries help you to find elements in the same way that users will find them. These queries allow you to find elements by their role, label, placeholder, text contents, display value, alt text, title, test ID.

That's actually in the order of recommendation."

The priority is even documented here:
https://testing-library.com/docs/queries/about/#priority

That's why I said that others should be preferred.

I'm fine with using test ids if others are not possible / practical for some reason.

There is no way to reliably select an element using title, label, role, placeholder, text contents, display value or alt text. Role is only one of those that is not changed too often. But role alone isn't enough. Therefore, only option is to select with test ID. I would even use plain old data-attributes if testing-library wouldn't limit us.

Needs to be said again, I'm not interested in the tools and how we use them. Tool (here. testing-library) doesn't affect my mind in anyway. If the tool is not solving the problem, then we change the tool. I'm more interested about the problem that we are trying to solve. In this case it is:

How do we test something that user can act with, in a way that:

we can minimize amount of unit tests that need to adapt for unrelated change

we can change our mind in the content

we can even change our mind on the element (e.g. could be button now, but in the future we decide that it's a link)

we can be sure enough that user find's the element (doesn't have to be same way, since it's just a burden. As long as abstraction levels are correct in the unit tests, we should be fine)

Clearly it's possible to reliably select an element using title, label, role, placeholder, text contents, display value or alt text.

Usually it's good to follow library's best practises.

How? Could you elaborate how you would solve the problem that I introduced in previous message?

One could argue that changing the title is not an unrelated change. I wouldn't find it unreasonable to have to update the test if the title changed. I'd even expect to have to change the test if the element changed. The title and element shouldn't be changing that often that it is a huge burden to have to update the test too. In fact, having to update tests in this situation has the advantage of keeping you aware of them, their implementations, and how they drive development...

src/behaviours/add-cluster/navigation-using-application-menu.test.ts

jim-docker · 2022-04-01T23:01:37Z

src/behaviours/add-cluster/navigation-using-application-menu.test.ts

+    expect(actual).toBeNull();
+  });
+
+  describe("when navigating to add cluster using application menu", () => {


Do the two describes here both match the same snapshot? How do they get different results when looking for "add-cluster-page"?

First snapshot test in this suite is snapshot that will match to "front page" of the application. However, currently it's just component that doesn't render any page component. Second snapshot matches to "add-cluster-page".

it("shows add cluster page", () => { tests that we are actually on that page and

it("renders", () => { tests how it's rendered at the moment. Covers a lot, since all of the sub components are rendered too and if there is change, it will break the snapshot and make sure you are attempting to change functionality in this page.

This is just a behaviour for navigating to that page. In future, there will be a lot more behaviours for "add-cluster-page" which will cover all of the functionalities. Most of these will be covered in other behaviours that actually do some action.

Examples of behaviours:

Add single cluster

Add multiple clusters

Cancel

Navigate using application menu

Navigate from catalog (most likely covered with all other behaviours)

ah, sorry, missed that there are two snapshots in the snap file.

This reverts commit ecd881c25df219b511d7efd43dac59e748b21cc7. Signed-off-by: Iku-turso <mikko.aspiala@gmail.com> Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

jim-docker

Certainly much careful hard work has gone into this and I'm confident that it can lead to a more stable and robust product. Although I still have some things to figure out before I'm comfortable with this di-based test framework. Lots to work by example from.

I think test names are suitable as documentation written as a user story...for users. I don't find the test implementations themselves that well documented, or easy to understand (I assume it comes with time). Also, test names are susceptible to staleness like code comments, they don't always get changed when the code changes.

Good work @jansav and @Iku-turso!

jim-docker · 2022-04-04T15:51:51Z

src/behaviours/add-cluster/navigation-using-application-menu.test.ts

+    expect(actual).toBeNull();
+  });
+
+  describe("when navigating to add cluster using application menu", () => {


ah, sorry, missed that there are two snapshots in the snap file.

jim-docker · 2022-04-06T22:33:54Z

src/behaviours/navigate-to-extension-page.test.tsx

+
+    describe("when changing page parameters", () => {
+      beforeEach(() => {
+        const button = rendered.getByTestId("button-to-change-page-parameters");


One could argue that changing the title is not an unrelated change. I wouldn't find it unreasonable to have to update the test if the title changed. I'd even expect to have to change the test if the element changed. The title and element shouldn't be changing that often that it is a huge burden to have to update the test too. In fact, having to update tests in this situation has the advantage of keeping you aware of them, their implementations, and how they drive development...

Iku-turso changed the title ~~Behavioural unit tests for navigation~~ Behavioural unit tests for navigation and reference Mar 23, 2022

Iku-turso force-pushed the behavioural-unit-tests-for-navigation branch from 438044f to 8cc73f0 Compare March 23, 2022 16:55

Iku-turso mentioned this pull request Mar 24, 2022

Technical requirements for behavioural unit tests #5084

Merged

jansav force-pushed the behavioural-unit-tests-for-navigation branch from 8cc73f0 to 0528cd7 Compare March 28, 2022 05:35

jansav changed the base branch from master to technical-requirements-for-behavioural-tests March 28, 2022 05:35

jansav added the area/tests label Mar 28, 2022

jansav added this to the 5.5.0 milestone Mar 28, 2022

Iku-turso force-pushed the technical-requirements-for-behavioural-tests branch from 075a647 to 799e43b Compare March 28, 2022 07:48

jansav force-pushed the behavioural-unit-tests-for-navigation branch from 0528cd7 to bb35372 Compare March 28, 2022 11:43

panuhorsmalahti reviewed Mar 28, 2022

View reviewed changes

Iku-turso force-pushed the technical-requirements-for-behavioural-tests branch from 799e43b to 0835fa6 Compare March 28, 2022 12:51

github-actions bot added the PR: needs rebase label Mar 28, 2022

Iku-turso force-pushed the behavioural-unit-tests-for-navigation branch from bb35372 to 9427de6 Compare March 28, 2022 13:11

github-actions bot removed the PR: needs rebase label Mar 28, 2022

Iku-turso force-pushed the behavioural-unit-tests-for-navigation branch from 9427de6 to b3f5ae6 Compare March 28, 2022 13:15

jansav force-pushed the behavioural-unit-tests-for-navigation branch 5 times, most recently from 1a767e8 to da94d9d Compare March 29, 2022 06:35

Iku-turso force-pushed the technical-requirements-for-behavioural-tests branch from 9352219 to 3c86646 Compare March 29, 2022 10:57

Iku-turso force-pushed the behavioural-unit-tests-for-navigation branch from da94d9d to 3ccf237 Compare March 29, 2022 11:03

jansav force-pushed the behavioural-unit-tests-for-navigation branch 5 times, most recently from df2e171 to 104ce24 Compare March 30, 2022 10:26

jansav force-pushed the behavioural-unit-tests-for-navigation branch from 0d39b23 to 21e5ab9 Compare March 31, 2022 06:11

jansav force-pushed the technical-requirements-for-behavioural-tests branch from 89d8a3b to 4a3ef18 Compare March 31, 2022 07:16

jansav force-pushed the behavioural-unit-tests-for-navigation branch 2 times, most recently from 22838a7 to 6083077 Compare March 31, 2022 07:24

jansav force-pushed the technical-requirements-for-behavioural-tests branch from 0c72746 to 7915e04 Compare March 31, 2022 12:16

jansav force-pushed the behavioural-unit-tests-for-navigation branch from 6083077 to adbe73c Compare March 31, 2022 12:16

Base automatically changed from technical-requirements-for-behavioural-tests to master March 31, 2022 13:57

Nokel81 force-pushed the behavioural-unit-tests-for-navigation branch from adbe73c to 63434bb Compare March 31, 2022 15:01

Nokel81 marked this pull request as ready for review March 31, 2022 15:01

Nokel81 requested a review from a team as a code owner March 31, 2022 15:01

Nokel81 requested review from jansav and Nokel81 and removed request for a team March 31, 2022 15:01

jansav requested review from aleksfront, DmitriyNoa, ixrock, jakolehm, jim-docker, jweak, nevalla and panuhorsmalahti April 1, 2022 05:26

panuhorsmalahti reviewed Apr 1, 2022

View reviewed changes

jim-docker reviewed Apr 4, 2022

View reviewed changes

Introduce a collection of behavioural unit tests for navigation

bb1be0a

This reverts commit ecd881c25df219b511d7efd43dac59e748b21cc7. Signed-off-by: Iku-turso <mikko.aspiala@gmail.com> Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

jansav force-pushed the behavioural-unit-tests-for-navigation branch from f5594fe to bb1be0a Compare April 4, 2022 05:21

jim-docker approved these changes Apr 6, 2022

View reviewed changes

jansav approved these changes Apr 12, 2022

View reviewed changes

jansav merged commit 55d8e0e into master Apr 12, 2022

jansav deleted the behavioural-unit-tests-for-navigation branch April 12, 2022 04:39

Nokel81 mentioned this pull request May 4, 2022

release v5.5.0-beta.0 #5326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Behavioural unit tests for navigation and reference #5085

Behavioural unit tests for navigation and reference #5085

Iku-turso commented Mar 23, 2022 •

edited

Loading

jansav commented Mar 28, 2022

panuhorsmalahti Mar 28, 2022

jansav Mar 28, 2022 •

edited

Loading

panuhorsmalahti Apr 1, 2022

Nokel81 Apr 1, 2022

jansav Apr 1, 2022

panuhorsmalahti Apr 4, 2022

jansav Apr 4, 2022

panuhorsmalahti Apr 4, 2022

github-actions bot commented Mar 28, 2022

github-actions bot commented Mar 28, 2022

panuhorsmalahti Apr 1, 2022 •

edited

Loading

jansav Apr 2, 2022

panuhorsmalahti Apr 4, 2022

jansav Apr 4, 2022 •

edited

Loading

panuhorsmalahti Apr 4, 2022

jansav Apr 4, 2022

jim-docker Apr 6, 2022

jim-docker Apr 1, 2022

jansav Apr 4, 2022

jim-docker Apr 4, 2022

jim-docker left a comment

jim-docker Apr 4, 2022

jim-docker Apr 6, 2022

Behavioural unit tests for navigation and reference #5085

Behavioural unit tests for navigation and reference #5085

Conversation

Iku-turso commented Mar 23, 2022 • edited Loading

jansav commented Mar 28, 2022

Choose a reason for hiding this comment

jansav Mar 28, 2022 • 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

github-actions bot commented Mar 28, 2022

github-actions bot commented Mar 28, 2022

panuhorsmalahti Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansav Apr 4, 2022 • 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

jim-docker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iku-turso commented Mar 23, 2022 •

edited

Loading

jansav Mar 28, 2022 •

edited

Loading

panuhorsmalahti Apr 1, 2022 •

edited

Loading

jansav Apr 4, 2022 •

edited

Loading