e2e: add FlashList tests #216

ElviraBurchik · 2022-03-22T17:13:11Z

Description

Adds two test cases:

Twitter example built with FlashList looks the same on a snapshot
Twitter example built with FlashList looks the same as the one with FlatList

I still need to resolve two issues:

saving diff if snapshots differ
Android snapshots doesn't look the same as emulator - created an issue to fix it later, since we don't run e2e tests on Android on CI it can wait

Reviewers’ hat-rack 🎩

Run test locally, confirm that it works with dev run-e2e-ios
See that android fails with dev run-e2e-android, but visual diff is saved to diff folder
Delete e2e/artifacts folde, run tests - reference artifacts should be created again

Screenshots or videos (if needed)

Checklist

I have added a changelog entry following the Keep a Changelog guidelines.

fortmarek

Some initial comments. I think most importantly we should work on how to write these tests with as minimal code as possible, so we can add them easily in the future.

fixture/android/app/src/main/AndroidManifest.xml

fixture/e2e/flashList.test.e2e.js

fortmarek · 2022-03-23T16:48:18Z

fixture/e2e/flashList.test.e2e.js

+    const testRunScreenshotPath = await element(
+      by.id("FlashList")
+    ).takeScreenshot(flashTwitterReferenceName);
+
+    // Path where we want to save the screenshot
+    const flatListReferencePath = path.resolve(
+      testArtifactsLocation,
+      flashTwitterReferenceName
+    );
+
+    // If reference is already there, compare the two screenshots
+    if (fs.existsSync(flatListReferencePath)) {
+      console.log(`testRunScreenshotPath ${testRunScreenshotPath}`);
+      console.log(`flatListReferencePath ${flatListReferencePath}`);
+      // compare screenshots, get difference
+      const diffPNG = pixelDifference(
+        testRunScreenshotPath,
+        flatListReferencePath
+      );
+
+      // If there is difference, fail the test
+      if (diffPNG !== null) {
+        saveDiff(diffPNG, "flash_twitter_looks_the_same_diff", platform);
+
+        throw new Error(
+          "There is difference between reference screenshot and test run screenshot"
+        );
+      }
+    } else {
+      // Save reference screenshot cause it doesn't exist yet
+      fs.renameSync(
+        testRunScreenshotPath,
+        flatListReferencePath,
+        function (err) {
+          if (err) throw err;
+        }
+      );
+      console.log("Reference screenshot created");


I'd love if this was extracted into a simple method such as:

assertSnapshot(matching: element, record: boolean) // not sure what type `element` is

We can also change it in a way to test that it's the same as before and the same as FlatList:

assertSnapshots(elementA: element, elementB: element) // this can probably be "recorded" every time

For reference, I am taking inspiration from this Swift library as I liked its API quite a lot back when I used it.

+1
We need to provide a simple API to write new tests. assert should only require an element and abstract out path related complexity. We should only need to worry about getting to element. Record is also helpful when we want to easily update things.

assertSnapshots is also great and in cases where both elements are not there on the screen at the same time there could be helpers like captureElement and it's output can also go to assertSnapshots.

fortmarek · 2022-03-23T16:49:01Z

fixture/e2e/flashList.test.e2e.js

+        function (err) {
+          if (err) throw err;
+        }


usage of arrow function would be easier to read here imho

fortmarek · 2022-03-23T16:49:55Z

fixture/e2e/flashList.test.e2e.js

+        );
+      }
+    } else {
+      // Save reference screenshot cause it doesn't exist yet


How do you re-take screenshots? Do you need to delete the file? I think a record boolean that tells you whether to take the snapshot again would make sense (as mentioned below)

I created reference snapshots and committed them to the repo. Now every test run compares new screenshots with reference once and asserts if they don't match with saving a diff snapshot. There is no need to delete anything in regular cases, when do you think it makes sense to record them again?

There is no need to delete anything in regular cases, when do you think it makes sense to record them again?

We will have to re-run screenshots every time we change the fixture. Additionally, we will also need to re-run them when we upgrade the simulators/emulators we run the screenshots on (it happens quite often things change with the OS version upgrade).

We should into making this as seamless as possible - deleting screenshots manually from the file system is imho tedious and error-prone. I believe we should unlock this functionality from code instead.

Right, then if it's in code updating the snapshots gonna look the following:

change the code to use record = true

run tests, re-record snapshots

change the code back?

Not cool.

I can add a dev command instead to delete the artifacts and run tests to generate new snapshots instead. What do you think?

Not cool.

I actually don't mind that set up. The big benefit is that you don't need to switch context when you want to re-record something. Having a dev command means going to command line, copy-paste the test name, and run the command. When you write a new test, you might want to re-record quite often which is why it's imho worth it to have it in code as it's so much easier to use. With the extra command, you would have to delete the screenshot after each run.

The biggest drawback is that you might forget to delete it - we can create an extra lint rule if that becomes a problem. But I think the pros outweigh the cons in this case. But I'm sure we're not the first creating such snapshot tests via detox - are there any other opensource or Shopify alternatives that we can inspire ourselves from?

I can add a dev command instead to delete the artifacts and run tests to generate new snapshots instead. What do you think?

What do I do if I want to delete a specific artifact? I suppose we could add possibility to specify the file name but if we abstract the artifact name to be derived from the test name, it might be tedious.

Additionally, we should probably avoid writing custom dev commands as we will most likely move away from dev.yml in the future as outside contributors won't be able to use it once we opensource the repo (that being said, we can still define custom commands in package.json).

Honestly, modifying the code only to make it re-record snapshot and then changing back is longer then manually deleting the folder corresponding to the test case. I will take a look into existing solutions, but having a record options doesn't thrill me for now.

Honestly, modifying the code only to make it re-record snapshot and then changing back is longer then manually deleting the folder corresponding to the test case

Could you explain why it is longer? But yeah, if you find some other examples for this, let me know! Curious to hear what @naqvitalha thinks about this, too.

fortmarek · 2022-03-23T16:51:28Z

fixture/e2e/flashList.test.e2e.js

+
+  it("Twitter with FlashList looks the same", async () => {
+    const testArtifactsLocation = ensureArtifactsLocation(
+      `twitter_with_flash_list`,


we can optionally infer the name from the test name as mentioned here

additionally, I don't think this needs to be in backticks or?

Good point 👍

fortmarek · 2022-03-23T16:52:56Z

fixture/e2e/DetoxHelpers.js

+    });
+  } else {
+    // enter demo mode
+    execSync("adb shell settings put global sysui_demo_allowed 1");


I would personally prefer await exec since the method is async anyway but probably won't make much difference

fortmarek · 2022-03-23T16:54:18Z

fixture/e2e/DetoxHelpers.js

+export function pixelDifference(
+  referencePath: String,
+  toMatchPath: String
+): PNG | null {


can this file be in typescript as I see you are declaring the types here?

naqvitalha

We need an easier API and that's all that I see that we need to really change.

naqvitalha · 2022-03-23T19:20:34Z

fixture/e2e/flashList.test.e2e.js

+    const testRunScreenshotPath = await element(
+      by.id("FlashList")
+    ).takeScreenshot(flashTwitterReferenceName);
+
+    // Path where we want to save the screenshot
+    const flatListReferencePath = path.resolve(
+      testArtifactsLocation,
+      flashTwitterReferenceName
+    );
+
+    // If reference is already there, compare the two screenshots
+    if (fs.existsSync(flatListReferencePath)) {
+      console.log(`testRunScreenshotPath ${testRunScreenshotPath}`);
+      console.log(`flatListReferencePath ${flatListReferencePath}`);
+      // compare screenshots, get difference
+      const diffPNG = pixelDifference(
+        testRunScreenshotPath,
+        flatListReferencePath
+      );
+
+      // If there is difference, fail the test
+      if (diffPNG !== null) {
+        saveDiff(diffPNG, "flash_twitter_looks_the_same_diff", platform);
+
+        throw new Error(
+          "There is difference between reference screenshot and test run screenshot"
+        );
+      }
+    } else {
+      // Save reference screenshot cause it doesn't exist yet
+      fs.renameSync(
+        testRunScreenshotPath,
+        flatListReferencePath,
+        function (err) {
+          if (err) throw err;
+        }
+      );
+      console.log("Reference screenshot created");


+1
We need to provide a simple API to write new tests. assert should only require an element and abstract out path related complexity. We should only need to worry about getting to element. Record is also helpful when we want to easily update things.

assertSnapshots is also great and in cases where both elements are not there on the screen at the same time there could be helpers like captureElement and it's output can also go to assertSnapshots.

fortmarek

Looking much better 👏 I have not done much research whether such a library exists but seems like it's general enough that we can potentially think of extracting it out in the future 👀

fortmarek · 2022-03-24T16:42:27Z

fixture/e2e/flashList.test.e2e.js

+    ).takeScreenshot(testName);
+
+    if (referenceExists(testName)) {
+      assertSnapshot(testRunScreenshotPath, testName);
+    } else {
+      saveReference(testRunScreenshotPath, testName);
+    }


much better 👏

Can we extract this into a separate and reusable method as well?

We can, but I don't want to do it for now - I'm not 100% happy with this approach it might be changed soon. Right now I'd prefer it to be super obvious what's happening for a person reading the test case.

fortmarek · 2022-03-24T16:45:01Z

fixture/src/Detox/SnapshotAsserts.ts

+    saveDiff(diffPNG, `${testName}_diff.png`);
+
+    throw new Error(
+      "There is difference between reference screenshot and test run screenshot"


Can we output the path of the file?

Good idea, done

fixture/src/Detox/SnapshotLocation.ts

fortmarek · 2022-03-24T16:47:05Z

fixture/src/Detox/SnapshotAsserts.ts

+
+import { pixelDifference } from "./PixelDifference";
+
+export const assertSnapshot = (snapshotPath: string, testName: string) => {


I'd probably change the snapshotPath to element and take the snapshot inside the file. I don't see a need to enforce users of the API to do so or?

fortmarek · 2022-03-24T16:48:23Z

fixture/e2e/flashList.test.e2e.js

+
+  it("Twitter with FlashList looks the same", async () => {
+    // TODOs: Get it from Jest
+    // expect.getState().currentTestName - doesn't  work


is it undefined or?

getState is undefined.

Based on this Github issue expect.getState().currentTestName is not supposed to work at all. It has been working, but it's accidental?

fortmarek · 2022-03-24T16:50:39Z

fixture/e2e/flashList.test.e2e.js

+    ).takeScreenshot(flatTwitterReferenceName);
+
+    if (!referenceExists(testName)) {
+      saveReference(testRunScreenshotPath, testName);


I'd put the referenceExists check into the saveReference method

It wouldn't work for the first test case then

how so? I don't see why it should not work for the first test case, too.

I see now - then we can run saveReference inside assertSnapshot instead?

fortmarek · 2022-03-24T16:53:01Z

fixture/e2e/flashList.test.e2e.js

+    const flashListReference = referenceExists(
+      "Twitter with FlashList looks the same"
+    );


This makes this test dependent on the Twitter with FlashList looks the same test - I don't think that's the best practice.

I'd either:

combine these two tests into one, so you can ensure FlashList screenshot is always taken

keep these tests separate and extract taking the FlashList screenshot into a helper method. We can then run it in both tests

Yeah, I don't like either

I'm not fond of the first one, because in this case we will always have a fresh screenshot of FlashList which might differ from reference and we might not know about it. The reference is supposed to be source of truth.

With the second one, getting the FlashList reference is already a one line
const reference = referenceExists("Twitter with FlashList looks the same");

the best I can do here is to move the test name constant out.

because in this case we will always have a fresh screenshot of FlashList which might differ from reference and we might not know about it

Why is that the case? You can always check whether the reference exists and only take a new screenshot when it does not.

With the second one, getting the FlashList reference is already a one line const reference = referenceExists("Twitter with FlashList looks the same");

Yeah, but the issue is you still take the screenshot in a different test, coupling those tests together. We should avoid that.

fortmarek

Just minor comments. I think we can make the API to assert snapshots just a little bit 🤏 better but it's already pretty convenient. Let me know @ElviraBurchik if you wanted to pair on it to finish it off 🏁

fortmarek · 2022-03-25T11:30:38Z

fixture/e2e/config.json

+  "verbose": true,
+  "preset": "react-native",
+  "transformIgnorePatterns": [
+    "<rootDir>/node_modules/(?!((jest-)?react-native|@react-native(-community)?)/)"


Based on our pairing session, I don't think this is necessary anymore.

fortmarek · 2022-03-25T11:34:01Z

fixture/e2e/flashList.test.e2e.js

+    if (referenceExists(flashListReferenceTestName)) {
+      assertSnapshot(testRunScreenshotPath, flashListReferenceTestName);
+    } else {
+      saveReference(testRunScreenshotPath, flashListReferenceTestName);


I remember from swift-snapshot-testing library that they failed the test when you were creating a new test. It does make kind of sense but you might want to check out the approach of jest-image-snapshot, too.

fortmarek · 2022-03-25T11:35:05Z

fixture/e2e/flashList.test.e2e.js

+    ).takeScreenshot(flatTwitterReferenceName);
+
+    if (!referenceExists(testName)) {
+      saveReference(testRunScreenshotPath, testName);


I see now - then we can run saveReference inside assertSnapshot instead?

fortmarek · 2022-03-25T11:35:50Z

fixture/e2e/flashList.test.e2e.js

+      assertSnapshots(flatListReference, flashListReference, testName);
+    } else {
+      throw new Error(
+        "One of the references doesn't exist. Please run the tests again."


we can also probably extract this out to assertSnapshots. It would be also helpful to know which references don't exist.

fortmarek · 2022-03-25T11:37:54Z

fixture/src/Detox/SnapshotAsserts.ts

+    throw new Error(
+      `There is difference between reference screenshot and test run screenshot. See diff: ${diffPath}`
+    );


wouldn't it be better to fail the test instead of throwing an error?

ElviraBurchik · 2022-03-25T14:01:14Z

fixture/src/Detox/SnapshotAsserts.ts

+  testName: string
+) => {
+  if (!firstPath) {
+    throw new Error(


Throwing an error here, because Jest's fail function is not available - jestjs/jest#11698

fortmarek

Just a couple of nits from my side, but I think we can more-or-less merge it now ✅ well done, Elvira, the API is almost perfect now 👏

fortmarek · 2022-03-25T15:30:29Z

fixture/e2e/flashList.test.e2e.js

+    assertSnapshot(testRunScreenshotPath, testName);
+
+    // Assert that FlatList reference is the same as with FlashList
+    assertSnapshotsEqual(
+      referenceExists(flashListReferenceTestName),
+      referenceExists(testName),
+      testName
+    );


this API is really nice ❤️

fortmarek · 2022-03-25T15:32:27Z

fixture/src/Detox/SnapshotLocation.ts

+  console.log(`Reference screenshot for test name "${testName}" was created`);
+};
+
+export const referenceExists = (testName: string): string | null => {


I find this misleading - we can just call it reference instead? referenceExists implies to me it will return a boolean instead of path to the image.

fair, it used to return boolean thus the name, but now it's indeed misleading

fortmarek · 2022-03-25T15:38:39Z

fixture/e2e/flashList.test.e2e.js

+  });
+
+  it("Twitter with FlatList looks the same as with FlashList", async () => {
+    // TODOs: Get it from Jest


I think we can just remove this.

fortmarek · 2022-03-25T15:40:08Z

fixture/src/Detox/SnapshotAsserts.ts

+) => {
+  if (!firstPath) {
+    throw new Error(
+      "First screenshot path is null. Please make sure that you have a screenshot before running this assertion."


Let's put the actual firstPath value here, too (same for secondPath)

- Remove TODO - Update errors to include invalid path

ElviraBurchik requested review from davebcn87, naqvitalha and fortmarek March 22, 2022 17:13

ElviraBurchik self-assigned this Mar 22, 2022

ElviraBurchik force-pushed the e2e/add-flash-list-tests branch from 92716f9 to 3f1bc96 Compare March 23, 2022 09:57

ElviraBurchik marked this pull request as ready for review March 23, 2022 13:59

fortmarek reviewed Mar 23, 2022

View reviewed changes

naqvitalha reviewed Mar 23, 2022

View reviewed changes

ElviraBurchik force-pushed the e2e/add-flash-list-tests branch 2 times, most recently from ba7b71e to 5d28f08 Compare March 24, 2022 14:47

fortmarek reviewed Mar 24, 2022

View reviewed changes

ElviraBurchik force-pushed the e2e/add-flash-list-tests branch 2 times, most recently from c086039 to d28bd69 Compare March 25, 2022 08:21

fortmarek reviewed Mar 25, 2022

View reviewed changes

ElviraBurchik commented Mar 25, 2022

View reviewed changes

ElviraBurchik added 12 commits March 25, 2022 16:02

WIP: FlashList e2e tests

b274ffe

Update tests

48e9027

Update tests and idle policy

7771bf6

Clean up

71964de

Convert DetoxHelpers to typescript

54a7eb4

Update test API

501797a

Address PR comments

c55f18f

Resolve merge conflicts

b0eb5cb

Clean up

9a9d4ac

Fix lint issues

f978888

Update asserts API

9d701ae

Fix lint issue

0e1fdc2

ElviraBurchik force-pushed the e2e/add-flash-list-tests branch from e46c21f to 0e1fdc2 Compare March 25, 2022 15:02

fortmarek approved these changes Mar 25, 2022

View reviewed changes

- Rename referenceExists to reference

6dd6563

- Remove TODO - Update errors to include invalid path

ElviraBurchik merged commit eff6d4e into main Mar 25, 2022

ElviraBurchik deleted the e2e/add-flash-list-tests branch March 25, 2022 17:02


		import { pixelDifference } from "./PixelDifference";

		export const assertSnapshot = (snapshotPath: string, testName: string) => {

e2e: add FlashList tests #216

e2e: add FlashList tests #216

Conversation

ElviraBurchik commented Mar 22, 2022 • edited Loading

Description

Reviewers’ hat-rack 🎩

Screenshots or videos (if needed)

Checklist

fortmarek 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

fortmarek Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek Mar 24, 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

Choose a reason for hiding this comment

naqvitalha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek 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

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

fortmarek 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

fortmarek 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

fortmarek Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

ElviraBurchik commented Mar 22, 2022 •

edited

Loading

fortmarek Mar 24, 2022 •

edited

Loading

fortmarek Mar 24, 2022 •

edited

Loading

fortmarek Mar 25, 2022 •

edited

Loading