Loop 2999 restructure #69

ginnyyadav · 2021-02-05T00:13:17Z

part 1

-restructured where the error tests live

import {test} and {config} seperately.
the tests themselves have stayed the same otherwise and the two error tests do run and pass.
I also deleted the G6 screen tests. not only were they not being run, but they provide no value. We cannot connect a G6 with automation, so there's no reason to go through only a tiny portion of the workflow
I renamed the status screen to homeScreen and its decedents to better descriptions of what they are glucose -> predicted glucose (since that's what its called)... and moved files around. I didn't want to commit them yet to avoid confusion, but alas they came up when I published the branch.

note: this PR will not be merged yet, instead since there is a lot of restructuring we are slowly creating intermediate pull requests until its finished state.

nhamming · 2021-02-05T12:18:01Z

just a quick thought about the G6 screens. I agree they aren't helping currently, but I do wonder about injecting state into the test setup such that we could make Tidepool Loop believe a transmitter and sensor are connected and thus be able to test specific UI (like editing notifications... but we won't be able to test the trigger of a notification.... unless we make a mock transmitter... which was done for the mock pod so there is precedent).

nhamming

Liking the improved structure and naming. Added some thoughts about a few files that might be improved by continuing in this direction. Those do not need to be handled in this PR (unless you want to).

LGTM

nhamming · 2021-02-05T12:22:43Z

e2e/errors/pump_errors.spec.js

+    it('and check no error on status screen', async () => {
+        homeScreen = await test.OpenHomeScreen();
+        await expect(homeScreen.HeaderSection.PumpErrorLabel).toBeNotVisible();
+    });


should this also check that the Pump suspended and tap to resume status is displayed?

yes it absolutely should. I was thinking about going in and expanding these a bit. But I didn't want to do too much at once. If you're cool with it then I will definitely add a couple more check to make this a more complete test

nhamming · 2021-02-05T12:23:41Z

e2e/errors/pump_errors.spec.js

+  describe('generate occlusion error', () => {
+    let homeScreen;
+    let pumpScreen;
+    beforeAll(async () => {


I don't know if you want to make this test more involved, but when the pump is in these cases, it shouldn't be able to deliver a bolus.

good thought!

nhamming · 2021-02-05T12:26:18Z

src/screens/base/screen.js

+let _baseScreenTests = async function ({ openScreenFunc, skipClose = false }) {
+  let screen;
+  it('can open', async () => {
+    screen = await openScreenFunc();
+  });
+  it('has a Header', async () => {
+    await expect(screen.Header).toBeVisible();
+  });
+  it('has a Back Button', async () => {
+    await expect(screen.BackButton).toBeVisible();
+  });
+  if (!skipClose) {
+    it('can close', async () => {
+      await screen.BackButton.tap();


I remember you talked about moving tests out of screens, but I cannot remember if you were planning to do all of them and if you were planning to do that now. So just pointing this out.

yes I'm doing this slowly but surely, this file is actually already changed in my local, but it took quite a bit of finagling so i'll send that up as an additional change to this PR.

nhamming · 2021-02-05T12:28:21Z

src/screens/homeScreen/tests.js

+module.exports = {
+  homeScreen,
+  activeInsulinScreen,
+  activeCarbohydratesScreen,


Am I correct to assume that test, for example the activeCarbohydratesScreen tests will live here at some point?

Actually it will be the opposite. I'm planning on completely removing this file, removing all the tests from the screens and keeping all the tests in the E2E folder. I'll delete this file when I have finished moving everything over.

actually i should just rename e2e. to tests....

nhamming · 2021-02-05T12:30:17Z

src/test.js

@@ -1,5 +1,5 @@
 /* eslint-disable no-undef */
-const StatusScreen = require("./status/index").Screen;
+const HomeScreen = require("./screens/homeScreen/HomeScreen").Screen;


Just trying to think of a way to improve the current name of test.js, since that is pretty general and not all the tests live here.

I see that this prepares the test runner and I see that navigation is being handled. Should (can) those 2 things be separated and file names clarify that distinction?

Ohh i'm glad you mentioned this. I sat staring at this and utilities.js for a while as well as properties.js. thinking there needs to be a way to better organize this. I completely agree with you. Right now on my local machine no tests live there anymore. So what is this? and how is it different from utilities.js and properties.js?

As far as my brain can tell
-test. js:
exports class Test -> includes setup of loop (i.e. closed loop/open loop, language, data, ect.
This is used before every single test to prepare the loop environment for testing.
Then, as you said before, there's a ton of functions in there for navigation, which the setup uses, but are NOT exclusive to the setup.

I think they should be pulled apart. that's my first instinct. Perhaps something like LoopSetup and utilities for the functions,...but wait we already have a utilities! so what's that:

utilities.js:

another class being exported called Utilities(this is why its helpful to do class file naming in pascal, otherwise you don't know what's a class and what's not at a glance)

this is a bunch of helper functions for common tasks. like addConfiguredPump(), or addCarbohydrates()

this makes sense to me for the most part. I'm not entirely sure why it's a class and not just a bunch of exported functions, but maybe it was to make the importing/exporting easier? not sure...and we might need to rename this because utilities..again doesn't really tell me what's going on here..maybe...not sure what i'd rename it to though

So, what does this mean so far? that means if we keep utilities.js as is, and we rename test.js to something like LoopSetup.js and keep just the test preparation in there, we still have all this navigation stuff with no real home...However, a thought occurs to me, why is the specific screen navigation living outside of their screens? why doesn't

async OpenHomeScreen() { return this.homeScreen; }

live inside the HomeScreen? My thought is that is makes sense there. Then if you wanted to use that screen in your test...you just import the HomeScreen...or the PredictedGlucoseScreen..or whatever screen you're jimmying around with at the time.

SO that sounds like a plan, but wait we forgot some other friends that live in this directory...

properties.js - mapping screens to their settings. I'm not entirely sure of the full scope of what's going on here, honestly...i think i'll figure it out as I break things.

index.js - exports two classes from their files. not necessary and will be removed
action.js - specific detox actions, condensed and made easy to use in tests. i'm a fan.
match - detox matchers. it's an attempt to condense the detox element matching code. I see the use for this, so it's fine, we just have to make sure that whenever you want to match an element you have to use something out of this file and not straight from the detox docs. otherwise there would be a lot of code duplication. This can be handled in an updated ReadMe.

So, yep. This was a huge brain dump. let me know what you think

wow. i also realized that in the base folder there is screen.js ...which seems to be more helper functions and for example it has

async SwipeDownUntilVisible(labelToSee) { await action.SwipeDownUntilVisible(labelToSee); }

but actions.js has the same thing

async SwipeDownUntilVisible(desiredLabel) { await _swipeUntilVisible(desiredLabel, 'down'); },

Need to see why that duplication is there. if it's not necessary then all duplicated actions should be removed and should live inside the actions.js

nhamming · 2021-02-05T12:34:46Z

src/utilities.js

@@ -89,12 +89,12 @@ module.exports = class Utilities {
    await this.loadTherapySettings();
  }
  async addUnconfiguredPump() {
-    let statusScreen = await this._testApp.OpenStatusScreen();


This seems to be another file that is a catch all. Again not sure if it makes sense to rename or restructure, but thought Is should point out that until someone knows what is in utilities they probably don't know to look there for these things.

yes. left a nice brain dump comment above :)

ginnyyadav · 2021-02-05T15:51:25Z

just a quick thought about the G6 screens. I agree they aren't helping currently, but I do wonder about injecting state into the test setup such that we could make Tidepool Loop believe a transmitter and sensor are connected and thus be able to test specific UI (like editing notifications... but we won't be able to test the trigger of a notification.... unless we make a mock transmitter... which was done for the mock pod so there is precedent).

this would be damn cool.

ginnyyadav · 2021-02-19T17:45:50Z

Please see the new Readme file as it explains a lot of what is happening here. I've tried to cherry pick changes into commits that make sense. let me know if any of this doesn't make sense however and I'll be glad to explain.

ginnyyadav · 2021-02-20T17:16:55Z

I added a third stage of commits above. This should alow you to checkout this branch and be able to run the homescreen.accessibility.spec.js on ios13 using build 1.1.0-2246 or later.

nhamming

As I mentioned in one of my comments, I mostly skimmed this PR, since it seemed like large portions were just moving code from one file to another.

I'm liking all these refactor changes and the real check is that the test run.

LGTM

nhamming · 2021-02-22T13:36:37Z

README.md


-`BUILD_DIR=${PWD}/build/build-289/Build/Products CONFIG=iphone-se-2 NAME=error_1 npm run test_e2e`
+Do **not** install `applesimutils` from Homebrew. Use the custom, pre-built binary found at `bin/applesimutils`. 


Maybe add a note about the need for this is to allow critical alerts in a system modal.

nhamming · 2021-02-22T13:38:51Z

e2e/accessibility/homescreen.accessbility.spec.js

+      await expect(homeScreen.CGMPill).toBeVisible();
+    });
+    it('has a loop icon', async () => {
+      // TODO


hm.... I thought this already existed?

yes, it's the smoke test converted into this. the only difference is there are a couple more checks on elements in the home screen (i.e. we weren't checking for any of the bottom button bar elements, 1 of the charts, or the cgm/pump pill)

nhamming · 2021-02-22T13:39:46Z

e2e/errors/cgm.errors.spec.js

@@ -0,0 +1,77 @@
+const {


I'm guessing most of these changes are moving to a new file. As such, I'm just skimming.

yep. that you are right about

ginnyyadav added 3 commits February 4, 2021 19:08

lock down dependencies

42ac34d

remove and rename error tests

a2c68c6

renamed the error tests and moved to a different structure

c70a258

ginnyyadav requested a review from nhamming February 5, 2021 00:14

nhamming approved these changes Feb 5, 2021

View reviewed changes

ginnyyadav added 5 commits February 19, 2021 10:03

tidying up eslint

184b850

tidying up dependencies and added detox recorder

b0a8d06

get rid of tests folder so tests live in e2e folder

41ed55f

change test script to match new folder names

3bbe398

Update README.md

65b9b32

ginnyyadav requested a review from nhamming February 19, 2021 17:37

ginnyyadav added 5 commits February 19, 2021 12:39

rename e2e tests

1393b38

rename source folder to utilities

21f1f66

smoke test turned into accessibility test

76b20a2

new screens

0d262d6

new screen export file

2d358ed

ginnyyadav added 11 commits February 19, 2021 13:07

fix formatting and typos

826c9dc

turning off some rules we don't need

5df2cc4

translate error 1 test to new format

012a735

add some new matchers and actions

63c42ad

add ability to match by ids

f886d7a

add onboarding skip remove unescessary setup

89356e0

add onboarding

b290d8f

new prepare test functions

5fac8cf

add labels and reorganize enUS file

f6917b0

add pump sim screen

a6d93c3

Merge branch 'dev' into loop-2999-restructure

4e8bcd9

ginnyyadav added 2 commits February 20, 2021 12:09

granting a bit more freedom from eslint

f918da3

account for faceid vs touchid

36cb514

nhamming approved these changes Feb 22, 2021

View reviewed changes

ginnyyadav added 8 commits February 22, 2021 15:11

add clarity on why we need a custom version of applesimutils

4946ce1

put matchers in alphabetical order so they're easier to read

a5f6654

update and add screens

4f9adf1

remove unnecessary eslint rule

8a3a7af

add pump errors test

1b6ca41

add deliver bolus as functional test

3f29f1c

alphabetize and order screens

c28e18e

add wait for element not visible action

de81a13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loop 2999 restructure #69

Loop 2999 restructure #69

ginnyyadav commented Feb 5, 2021 •

edited

Loading

nhamming commented Feb 5, 2021

nhamming left a comment

nhamming Feb 5, 2021

ginnyyadav Feb 5, 2021

nhamming Feb 5, 2021

ginnyyadav Feb 5, 2021

nhamming Feb 5, 2021

ginnyyadav Feb 5, 2021 •

edited

Loading

nhamming Feb 5, 2021 •

edited

Loading

ginnyyadav Feb 5, 2021

ginnyyadav Feb 5, 2021

nhamming Feb 5, 2021

ginnyyadav Feb 5, 2021 •

edited

Loading

ginnyyadav Feb 5, 2021

nhamming Feb 5, 2021

ginnyyadav Feb 5, 2021

ginnyyadav commented Feb 5, 2021

ginnyyadav commented Feb 19, 2021

ginnyyadav commented Feb 20, 2021

nhamming left a comment

nhamming Feb 22, 2021

nhamming Feb 22, 2021

ginnyyadav Feb 22, 2021

nhamming Feb 22, 2021

ginnyyadav Feb 22, 2021


		`BUILD_DIR=${PWD}/build/build-289/Build/Products CONFIG=iphone-se-2 NAME=error_1 npm run test_e2e`
		Do not install `applesimutils` from Homebrew. Use the custom, pre-built binary found at `bin/applesimutils`.

Loop 2999 restructure #69

Are you sure you want to change the base?

Loop 2999 restructure #69

Conversation

ginnyyadav commented Feb 5, 2021 • edited Loading

nhamming commented Feb 5, 2021

nhamming 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

ginnyyadav Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

nhamming Feb 5, 2021 • 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

ginnyyadav Feb 5, 2021 • 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

ginnyyadav commented Feb 5, 2021

ginnyyadav commented Feb 19, 2021

ginnyyadav commented Feb 20, 2021

nhamming 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

ginnyyadav commented Feb 5, 2021 •

edited

Loading

ginnyyadav Feb 5, 2021 •

edited

Loading

nhamming Feb 5, 2021 •

edited

Loading

ginnyyadav Feb 5, 2021 •

edited

Loading