Move e2etest pages into rntester by kmelmon · Pull Request #5555 · microsoft/react-native-windows

kmelmon · 2020-07-18T03:14:32Z

This change prepares us to use RNTester as the primary app for end-to-end UI integration tests. Currently we use our own app inside E2ETest. This will limit us as we'd need to create test pages from scratch. We already have a lot of potential test coverage with RNTester, and we can add whatever pages we want to it as well, so let's use RNTester instead.

This change moves the 6 existing test pages over to RNTester and makes the necessary changes to the test app to load RNTester instead of the current app.

Microsoft Reviewers: Open in CodeFlow

…ake2

kmelmon · 2020-07-20T20:31:05Z

  "HorizontalAlignment": "Stretch",
  "Margin": "0,0,0,0",
-  "RenderSize": [800, 360],
+  "RenderSize": [759, 360],


759 [](start = 17, length = 3)

These smaller widths are expected. RNTester has a container component that has margins on it, and this pages stretches across the entire available space, which is now smaller. #Resolved

the e2etest app had some code on startup which set the window size to 800x600 I believe, to ensure we get a consistent size. We should do that here too, otherwise the masters may not match if people ever resize/maximize etc #Resolved

That code in the app side is still there. Also the tree dumper control has code to resize to 800x600.

In reply to: 457682484 [](ancestors = 457682484)

NickGerleman · 2020-07-20T20:34:14Z

    "strict": true,
-    "rootDir": "."
+    "rootDir": ".",
+    "esModuleInterop": true


This could be breaking for code relying on previous esModule CommonJS interop behavior (e.g. import * is not callable with this enabled). What led to the need for the change and is it possible to change imports/exports to get the same effect? Note that TS has some special syntax for CommonJS interop as well.

The rest of the packages in the repo have this turned off, so having consistent behavior would be nice. #Pending

All the tests were originally authored over in packages\e2etest with this turned on. So for example this line of code in AccessibilityTestPage.tsx won't compile without it turned on:

import React, { useState } from 'react'; => error TS1259: Module '"D:/rnw/node_modules/@types/react/index"' can only be default-imported using the 'esModuleInterop' flag

I honestly don't understand the error so I went the easy route and turned the option on. I'm happy to change the import if only I could understand, it's quite bizarre!

In reply to: 457675352 [](ancestors = 457675352)

Reading this article over: https://itnext.io/great-import-schism-typescript-confusion-around-imports-explained-d512fc6769c2

The author recommends turning this option on: "...So I really advocate the usage of esModuleInterop option, cause it not only allows you to write less code, easier to read and spec-complaint code (and it's not just ranting, rollup for example won't allow you to use star imports in such way), but also mitigates some friction between typescript and babel communities."

In reply to: 457727708 [](ancestors = 457727708,457675352)

Yeah, Babel having it on by default and Typescript having it off by default is annoying. I'd kind of rather we have it off everywhere or on everywhere personally, and the risk of runtime issues makes me hesitant to turn it on anywhere where it used to be off.

The root cause of the issue is that CommonJS is still used internally for React, and Node, but ES6 modules added default imports which don't really have a commonJS equivalent. The interop setting sort of changes how Typescript or Babel projects a default import. My own preference has been to just avoid default import/export with anything interfacing with CommonJS since behavior is settings dependent.

I think the non esModuleInterop way to do this would be to have "import * as React from 'react'" on its own line instead of the default import. #Resolved

Thanks for the tip. I turned the option off and fixed up all the imports to do it the "non-esModuleInterop" way. Would love to read an article that actually explains all of this, I feel like I came to the party after everyone else already is in the club, leaving me feeling rather clueless.

In reply to: 457742505 [](ancestors = 457742505)

I think the historical context of being used to CommonJS before ES6 helps. Finding an article explaining CommonJS, then looking at the ES6 article on MDN should give some context on the different models, and why default imports (or setting module.exports to a single function) are wonky.

The basic idea is that CommonJS modules export a single value. This is often an object that's an aggregate of exports, which maps directly to most of ES6, but ES6 default imports don't really have a CommonJS equivalent. Without esModuleInterop, a default import will look for the ".default" property of the CommonJS export, and agregate imports are callable, since CommonJS allows exporting single functions.

With interop on, idefault imports will import the entire aggregate object IIRC, and calling a CommonJS export requires doing a default import instead of importing all.

NickGerleman · 2020-07-20T20:35:12Z


 beforeAll(() => {
-  HomePage.backToHomePage();
+  //  HomePage.backToHomePage();


Could we leave a comment about why this is commented out? #Resolved

Thanks for the reminder. This code will never be necessary so I just removed it from all the tests.

In reply to: 457675832 [](ancestors = 457675832)

NickGerleman · 2020-07-20T20:37:17Z

    },
+    {
+      "type": "platform",
+      "file": "src/RNTester/js/examples-win/LegacyTests/AccessibilityTestPage.tsx"


If you want, you could also add "examples-win" to the list of unenforced directories at the top of the file and remove the overrides listed for it. Nothing in that directory should need three-way-merge, so it seems safe. #Resolved

done

In reply to: 457676898 [](ancestors = 457676898)

ghost · 2020-07-21T00:02:36Z

Hello @kmelmon!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

… test already launches the app

…on. Reordering items.

…e-launching the test app. Trying --no-launch option

* first batch of changes * Change files * lint * fix test failures, all due to margin within RNTester * move (?) files * refactor consts * remove unused strings * remove unreliable test * increase app launch wait, removed unnecessary code * exclude examples-win * turn off esModuleInterop * lint fix * experimenting with sleep before test starts * troubleshooting instability with first test: try --no-launch param as test already launches the app * test instability continued: issue may be due to FlatList virtualization. Reordering items. * revert --no-launch, this is unrelated * it looks like the first test reliability issue is caused by killing/re-launching the test app. Trying --no-launch option

Summary: This change adds a testID to each item in the RNTester list. This helps Appium find items in the list for automated tests. The change was added in react-native-windows as part of end-to-end test infrastructure changes. See microsoft/react-native-windows#5555 We'd like to remove this forked file, upstreaming this change will enable us to do that. ## Changelog  [General] [Changed] - add testID to items in RNTester test list, helps test automation tools find the items Pull Request resolved: #30138 Test Plan: Change is currently running in react-native-windows CI loop. Reviewed By: cpojer Differential Revision: D24434352 Pulled By: appden fbshipit-source-id: 998916d8fe4e4e4cd6ac764baabb9fd5f2e312c7

kmelmon added 8 commits July 17, 2020 20:12

first batch of changes

1981bd1

Change files

66724f0

lint

8f30b43

Merge remote-tracking branch 'upstream/master' into rntester-phase1-t…

58b3fb9

…ake2

fix test failures, all due to margin within RNTester

33e5e72

move (?) files

4be6f7d

refactor consts

d0a311c

remove unused strings

d6199e7

ghost added Area: Test Infrastructure Area: Tests labels Jul 20, 2020

remove unreliable test

40aa73f

kmelmon marked this pull request as ready for review July 20, 2020 20:27

kmelmon requested a review from a team as a code owner July 20, 2020 20:27

kmelmon commented Jul 20, 2020

View reviewed changes

NickGerleman approved these changes Jul 20, 2020

View reviewed changes

kmelmon added 4 commits July 20, 2020 15:04

increase app launch wait, removed unnecessary code

40604fb

exclude examples-win

5d115f8

turn off esModuleInterop

496928b

lint fix

4fefe67

kmelmon added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jul 21, 2020

kmelmon removed the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jul 21, 2020

kmelmon added 5 commits July 20, 2020 17:45

experimenting with sleep before test starts

0e93302

troubleshooting instability with first test: try --no-launch param as…

8f412c5

… test already launches the app

test instability continued: issue may be due to FlatList virtualizati…

ab474ae

…on. Reordering items.

revert --no-launch, this is unrelated

49f4a41

it looks like the first test reliability issue is caused by killing/r…

cc12cb3

…e-launching the test app. Trying --no-launch option

kmelmon merged commit 59a2dcf into microsoft:master Jul 21, 2020

NickGerleman mentioned this pull request Aug 25, 2020

Upstream change to add TestID to RNTester Buttons #5834

Closed

kmelmon mentioned this pull request Oct 8, 2020

add testID, helps tests find items in the list facebook/react-native#30138

Closed

Conversation

kmelmon commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

kmelmon Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asklar Jul 20, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

NickGerleman Jul 20, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

kmelmon Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

NickGerleman Jul 20, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

NickGerleman Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

NickGerleman Jul 20, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

NickGerleman Jul 20, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 21, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kmelmon commented Jul 18, 2020 •

edited

Loading

kmelmon Jul 20, 2020 •

edited

Loading

asklar Jul 20, 2020 •

edited by kmelmon

Loading

NickGerleman Jul 20, 2020 •

edited by kmelmon

Loading

NickGerleman Jul 20, 2020 •

edited by kmelmon

Loading

NickGerleman Jul 20, 2020 •

edited by kmelmon

Loading

NickGerleman Jul 20, 2020 •

edited by kmelmon

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.