Have Jest cover Android-only codepaths. #4700

chrisbobbe · 2021-04-27T15:58:01Z

Forking from @WesleyAC's comment at #4694 (comment), which prompted me to realize that it's past time to have Jest include coverage of codepaths with both 'ios' and 'android' for Platform.OS.

Both platforms will be covered in CI, but only one (iOS) will run by default in local testing, for speed. That can be changed by passing --platform both to tools/test.

See discussion here.

Also fix some issues with the replaceRevive tests.

gnprice

Thanks @chrisbobbe ! This will be great. Comments below.

src/boot/__tests__/replaceRevive-test.js

yarn.lock

jest.config.js

gnprice · 2021-04-27T17:49:55Z

tools/test

@@ -117,6 +117,10 @@ run_native() {
    )
 }

+run_native() {
+    run_native_android || return


The || return is redundant:

If the run_native_android command succeeds, the || won't be used.

If it fails, then:

The || will be used, and return with no arguments means we'll return the exit status of that command. (See help return or the entry here in the manual.)

If OTOH the whole command were just run_native_android, with no || return, then set -e already means that on failure the function would return with that exit code.

A lot of spots in this script have a pattern || return 0. That has the key difference that if the left-hand command fails, it makes the function return but with success (exit status 0). So it's used for conditions that mean there's no work to be done, like if there are no relevant files.

Huh, interesting! It looks like something strange is happening.

I added || return because, without it, I found that run_native_android could fail without causing run_native to exit immediately with the same exit code. (I caused a failure in run_native_android by putting garbage in one of our Kotlin files.)

I've tried a few things just now, and I think I found the culprit. From the Bash manual on set -e (emphasis mine),

The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test in an if statement, part of any command executed in a && or || list except the command following the final && or ||, any command in a pipeline but the last, or if the command’s return status is being inverted with !.

If I make this change just after the case "$suite" in block, in the for suite in "${suites[@]}" loop, I can then remove the || return and it works as expected (a run_native_android failure immediately causes run_native to exit with the same exit code):

- esac || failed+=($suite) + esac

🤯

Is this diagnosis right, and if so, how would you like to proceed? Maybe we could rewrite the conditional that controls whether failed+=($suite) runs.

Urgh, right. Thanks for catching that. This is the main gotcha in set -e, where it doesn't do everything you'd think it does (because any reasonable version of it absolutely would.) 🤯 is right on.

This also explains why I wrote a similar || return in one spot in the existing code, at check_node || return. When I wrote this comment I was wondering about that...

I guess the short of it is that inside all these functions, we effectively don't have the benefit of set -e. And we can't even re-enable it from inside each function, as this bit later in that same manual item explains:

If a compound command or shell function sets -e while executing in a context where -e is ignored, that setting will not have any effect until the compound command or the command containing the function call completes.

I think the right immediate fix is to use || return just as this branch does. Basically we need that wherever we have a nontrivial command that isn't the final command of its function. I think the existing code is all OK in this respect, thanks to that || return in one place we did need it.

That and maybe a comment on the set -e at the top, warning that it doesn't (and sadly can't) actually apply to most of the code.

Then... if we want to get back set -e behavior inside these functions, while still being able to "catch" its propagation at the outer loop so that we can go on to run the other suites, I think basically what's required is to fork a subshell (with the ( … ) syntax -- like we already do inside run_android in order to use cd and contain its effect.) I think that's the real point of what that quoted bit of docs is saying: set -e stops having any effect if it isn't going to be able to go all the way to exit the whole shell. That's a bit of overhead, but only like a millisecond each:

$ time bash -c 'for i in {1..100}; do ( : ); done' time: 0.099s wall (0.065s u, 0.041s s)

so not a problem at all. (That fast startup time is one of Bash's handful of great strengths, compared to almost any other language that doesn't require a compile step.)

OK, sounds good! Glad to see the 🤯 feeling is shared! 😅 I'll put a comment on the set -e.

I'm not sure what's the best strategy for forking a subshell where appropriate; we'd of course like to leave a good signpost if any run_foo functions we add in the future have to remember to do something.

tools/test

jest/jestSetup.js

src/boot/__tests__/ZulipAsyncStorage-test.js

jest/jestSetup.js

docs/howto/testing.md

gnprice · 2021-04-27T19:19:43Z

docs/howto/testing.md

+We've configured Jest to run two different
+["projects"](https://jestjs.io/docs/configuration#projects-arraystring--projectconfig):
+one with an iOS preset, and one with an Android preset.
+`tools/test jest` will run one or the other, or both, depending on
+what you pass for `--platform`.
+
+If it makes sense for a test file to be dedicated to just one
+platform, and that's unlikely to change, you can instead suffix it
+with `-test.ios.js` or `-test.android.js`, and it will be ignored when
+the other platform's project runs.


This feels more detailed about the Jest specifics than is needed here -- those details can be left to the code that controls them. (E.g. I think the concept of a Jest "project" can be left to jest.config.js and to that case block in tools/test that sets --selectedProjects.)

For this doc the ideal thing is to be more direct about what happens by default, and what you might want to do instead (and when.)

chrisbobbe · 2021-04-27T19:32:48Z

Thanks for the prompt review, @gnprice! Reading now.

We've had Jest 26 as a direct dependency for a while (since fb23341), so it's surprising that the `jest` command hasn't been using it. As Greg points out [0], that turns out to be because `jest-expo` apparently provides its own wrapper for the `jest` command, using the Jest that `jest-expo` depends on. That's Jest 25, even in `jest-expo`'s latest. And Yarn ends up giving us that wrapper. We know we'll soon want to use Jest 26 for a few things: - The "modern" implementation of fake timers [1], for zulip#4165. - The `selectProjects` CLI argument [2], for testing Android codepaths in a nice way (later in this series). But we've *also* realized that we don't really want to jettison `jest-expo` again, as we've done a few times as we learn new things: 62621ef jest: Add `jest-expo` preset, to be used in the next commit. 347aa96 jest: Back out `jest-expo` preset, for now. c4fca9d jest: Add and use `jest-expo` preset, again. In particular, we really want to use the `jest-expo/ios` and `jest-expo/android` presets (which we'll do later in this series). This is a hack because the `resolutions.jest-expo/jest` range `^26.4.1` is incompatible with the original range that `jest-expo` declares; that's `^25.2.0`. We believe this should get us a warning from Yarn; according to a doc [3], "You will receive a warning if your resolution version or range is not compatible with the original version range." I haven't seen a warning like that, even after removing node_modules and running `yarn`; this seems like a Yarn bug. I've opened expo/expo#12735 to bump Jest in the `jest-expo` project; this hack should hopefully be fine while we wait for that. [0] zulip#4700 (comment) [1] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2600 [2] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2610 [3] https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-tips-tricks

As Greg suggests at zulip#4700 (comment). These made sense before `tools/test` existed, and they were good shortcuts for complicated commands. Now, even if they still work, they're just obstacles in the way of getting familiar with `tools/test` and all its features.

This doesn't control which suites are run; it controls which files are tested. So, give it a name that makes more explicit what it does and doesn't do. As Greg suggests: zulip#4700 (comment).

chrisbobbe · 2021-04-29T00:29:05Z

Thanks again for the review! Revision pushed. I've also replied inline to some of your comments, and you've already responded to some.

As Greg explains; see zulip#4700 (comment).

And promote it in the release docs and run it in CI. As Greg suggests: zulip#4700 (comment).

WesleyAC

Thanks for doing this! I'm very happy that we'll be getting more coverage here. Just a few comments :)

WesleyAC · 2021-04-29T09:13:16Z

tools/test

+# """
+# The shell does not exit if the command that fails is [...] part of
+# any command executed in a `&&` or `||` list except the command
+# following the final `&&` or `||` [...].
+# """


nit: I would use:

> blockquote style

rather than the triple quotes, since I think that's clearer, and uses less vertical space.

WesleyAC · 2021-04-29T09:14:41Z

tools/test

@@ -1,4 +1,21 @@
 #!/usr/bin/env bash
+
+# Careful! This doesn't do everything you'd think it does. In fact, we


I would replace This here with 'set -e', since otherwise it's unclear what this comment is referring to until you read the entire thing.

WesleyAC · 2021-04-29T09:22:01Z

tools/test

+              Run tests as if on iOS, or Android, or both. The default,
+              "sloppy", takes a shortcut in some suites, so the tests
+              run faster but full coverage of both platforms isn't
+              guaranteed. In particular, while the vast majority of
+              our Jest tests don't depend meaningfully on the platform,
+              we don't yet have a clever way to identify the ones that
+              do. So, "sloppy" just picks one platform to simulate
+              (iOS). Don't use "sloppy" if you want coverage of the
+              other platform.


I think this could be clearer:

Run tests as if on iOS, or Android, or both. The default, "sloppy", takes a shortcut in some suites, so the tests run faster but full coverage of both platforms isn't guaranteed. Specifically, "sloppy" will run both Android and iOS native tests, but will run Jest tests on only one platform (iOS). This is usually fine, because the vast majority of our Jest tests don't depend meaningfully on the platform.

WesleyAC · 2021-04-29T09:24:17Z

tools/test

+        # This is where `sloppy` is sloppy: we just choose iOS, so the
+        # tests will run faster, but at the expense of a (relatively
+        # small) loss of coverage.
+        sloppy) jest_args+=( --selectProjects ios );;


A possibly dumb idea — we could pick whether it runs as iOS or Android randomly. The idea there being that if it's only failing on one, picking randomly would cause you to notice sooner during local development, rather than it only being caught in CI. And while that makes it flaky, it's a pretty reasonable type of flake, since it'll always be catching a real test failure.

Interesting; I kind of like this idea. @gnprice, what do you think?

Hmm, maybe. I like the idea of causing you to notice sooner. I think this would definitely best be done as a separate change.

WesleyAC · 2021-04-29T09:26:42Z

tools/test

@@ -82,6 +84,7 @@ while (( $# )); do
            esac
            shift
            ;;
+        --all) files=all; platform=both; shift;;


I think it someone does tools/test --all --platform=android or similar, the --platform will overwrite the --all, is that right?

Probably we should just fail if someone passes both --all and --platform or --diff?

Makes sense. I also wondered if we should write the first line of the usage string differently, to reflect that:

usage: tools/test [--all-files | --diff COMMIT] [--platform <ios|android|both|sloppy>] [--all] [SUITES]

(I.e., from reading that it looks like it's perfectly fine to do --all and --platform or --diff.)

But I'm not sure how best to do that while preserving readability. Maybe it's fine as-is.

Hmm, looking again, I'm not sure what's the best way to enforce these rules on what gets passed. (I see that we're already not enforcing that only one of --diff and --all-files gets passed.)

I'm not super familiar with Bash, yet. It seems plausible to me that the best way to start enforcing these things will be after we've started using getopt for something different; Greg suggested that here. Do you think that's right?

For --all-files and --diff, I think the behavior is actually fine: they're each setting the same option to different values, and the last one wins. That's a common CLI convention, and it can come in handy when you want to have some default command line (e.g. an alias in your interactive shell) and sometimes pass additional arguments, and sometimes override an option that was already in the default version of the command.

I think the way --all interacts with those two and --platform is also fine for the same reason. It means that, for example, you could have an alias that expands to tools/test --all, and effectively it'd be tools/test but with your custom choice of a different default -- you'd still be able to override it to get other behavior when you wanted.

(For the real thorough version of that, we'd also add a flag that means the default files=branch behavior as opposed to --all-files or --diff.)

If we did want behavior where some options are mutually exclusive, that's actually orthogonal to getopt; the way you'd implement it if we were using getopt is basically the same as how you'd implement it now.

WesleyAC · 2021-04-29T09:31:10Z

jest/jestSetup.js

+      compress: async (input: string) => `${header}${input}`,
+      decompress: async (input: string) => input.replace(header, ''),


This is fine as is, but I'd be a bit more comfortable with something that mangles the whole string. Possibly base64encoding it, in addition to adding the header? (I'm not sure what JS runtime our jest tests execute in, so I'm not sure if we have a reasonable base64 function easily available to us, but if we do, that seems like a good option).

Sounds good; there is such a thing: Buffer. This isn't available for JS that runs in an app build; it looks like we do use an NPM package called base-64 for that in some places.

The Jest tests execute in Node, and we currently recommend the latest 12.x and use that in CI (see f83ccc6 for what this means more concretely). #4263 is open to upgrade to 14.

WesleyAC · 2021-04-29T09:32:14Z

src/boot/__tests__/ZulipAsyncStorage-test.js

+// `ZulipAsyncStorage-test.android.js`, to target entire test suites
+// to Android and iOS.
+describe('ZulipAsyncStorage', () => {
+  describe('setItem', () => {


Can we test getItem as well?

Sure! My work on the getItem tests today has led to some reworking (and increased coverage) of the setItem tests, too. It's enough that I think it makes sense for that stuff to go in its own PR, so I'll take the setItem tests out of this PR and send a new PR that tests that and getItem.

gnprice

This looks great, thanks!

Other than the two small things below, and the things in threads above where I feel like I need to explore a bit in order to have a concrete suggestion and so I may try for myself later, I have just one other comment:

I think the series can be simplified somewhat by taking the three commits where --full gets renamed, --platform introduced, and we have both --all-files and --all, and rearranging to:

One commit renames --full directly to --all. After all, in the end that's what we want in all the known use cases of the old --full. At this stage it means the same thing as --all-files would, because there's no --platform yet.
Then in the commit that adds --platform, --all starts including the meaning of --platform both, and we introduce --all-files for the behavior that's only about the files variable.

In particular this means the places we mention --full change only in the first of those commits, instead of all three of the existing sequence.

tools/test

docs/howto/testing.md

We've had Jest 26 as a direct dependency for a while (since fb23341), so it's surprising that the `jest` command hasn't been using it. As Greg points out [0], that turns out to be because `jest-expo` apparently provides its own wrapper for the `jest` command, using the Jest that `jest-expo` depends on. That's Jest 25, even in `jest-expo`'s latest. And Yarn ends up giving us that wrapper. We know we'll soon want to use Jest 26 for a few things: - The "modern" implementation of fake timers [1], for zulip#4165. - The `selectProjects` CLI argument [2], for testing Android codepaths in a nice way (later in this series). But we've *also* realized that we don't really want to jettison `jest-expo` again, as we've done a few times as we learn new things: 62621ef jest: Add `jest-expo` preset, to be used in the next commit. 347aa96 jest: Back out `jest-expo` preset, for now. c4fca9d jest: Add and use `jest-expo` preset, again. In particular, we really want to use the `jest-expo/ios` and `jest-expo/android` presets (which we'll do later in this series). This is a hack because the `resolutions.jest-expo/jest` range `^26.4.1` is incompatible with the original range that `jest-expo` declares; that's `^25.2.0`. We believe this should get us a warning from Yarn; according to a doc [3], "You will receive a warning if your resolution version or range is not compatible with the original version range." I haven't seen a warning like that, even after removing node_modules and running `yarn`; this seems like a Yarn bug. I've opened expo/expo#12735 to bump Jest in the `jest-expo` project; this hack should hopefully be fine while we wait for that. [0] zulip#4700 (comment) [1] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2600 [2] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2610 [3] https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-tips-tricks

As Greg explains; see zulip#4700 (comment).

As Greg suggests at zulip#4700 (comment). These made sense before `tools/test` existed, and they were good shortcuts for complicated commands. Now, even if they still work, they're just obstacles in the way of getting familiar with `tools/test` and all its features.

chrisbobbe · 2021-04-30T21:55:07Z

Thanks for the review! Revision pushed.

I've removed addition of tests for ZulipAsyncStorage.setItem; I'll send that in a separate PR with tests for ZulipAsyncStorage.getItem as well (it started to make the most sense to split that stuff off into its own PR). EDIT: #4709

gnprice

Thanks @chrisbobbe ! Looks great -- tiny comments below.

I'm about to merge, so I'll just fix these along the way.

gnprice · 2021-05-03T20:43:11Z

tools/test

+# > The shell does not exit if the command that fails is [...] part of
+# any command executed in a `&&` or `||` list except the command
+# following the final `&&` or `||` [...].


nit: using > on each line makes it clearer this is all part of the quote

gnprice · 2021-05-03T20:43:58Z

tools/test

 files=branch
 fix=
+platform=sloppy
 suites=()


tiny nit: clearest if these are kept in the same order as the cases that operate on them, below

We've had Jest 26 as a direct dependency for a while (since fb23341), so it's surprising that the `jest` command hasn't been using it. As Greg points out [0], that turns out to be because `jest-expo` apparently provides its own wrapper for the `jest` command, using the Jest that `jest-expo` depends on. That's Jest 25, even in `jest-expo`'s latest. And Yarn ends up giving us that wrapper. We know we'll soon want to use Jest 26 for a few things: - The "modern" implementation of fake timers [1], for zulip#4165. - The `selectProjects` CLI argument [2], for testing Android codepaths in a nice way (later in this series). But we've *also* realized that we don't really want to jettison `jest-expo` again, as we've done a few times as we learn new things: 62621ef jest: Add `jest-expo` preset, to be used in the next commit. 347aa96 jest: Back out `jest-expo` preset, for now. c4fca9d jest: Add and use `jest-expo` preset, again. In particular, we really want to use the `jest-expo/ios` and `jest-expo/android` presets (which we'll do later in this series). This is a hack because the `resolutions.jest-expo/jest` range `^26.4.1` is incompatible with the original range that `jest-expo` declares; that's `^25.2.0`. We believe this should get us a warning from Yarn; according to a doc [3], "You will receive a warning if your resolution version or range is not compatible with the original version range." I haven't seen a warning like that, even after removing node_modules and running `yarn`; this seems like a Yarn bug. I've opened expo/expo#12735 to bump Jest in the `jest-expo` project; this hack should hopefully be fine while we wait for that. [0] zulip#4700 (comment) [1] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2600 [2] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2610 [3] https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-tips-tricks

As Greg explains; see zulip#4700 (comment).

As Greg suggests at zulip#4700 (comment). These made sense before `tools/test` existed, and they were good shortcuts for complicated commands. Now, even if they still work, they're just obstacles in the way of getting familiar with `tools/test` and all its features.

chrisbobbe · 2021-05-04T23:27:59Z

Thanks for the reviews!

I'm about to merge, so I'll just fix these along the way.

I've gone ahead and made the two targeted fixes for the nits you pointed out (and made no further changes), and pushed that to this branch, just in case that's helpful. 😛 (I haven't merged in case your hesitation is because you found a problem.)

We've had Jest 26 as a direct dependency for a while (since fb23341), so it's surprising that the `jest` command hasn't been using it. As Greg points out [0], that turns out to be because `jest-expo` apparently provides its own wrapper for the `jest` command, using the Jest that `jest-expo` depends on. That's Jest 25, even in `jest-expo`'s latest. And Yarn ends up giving us that wrapper. We know we'll soon want to use Jest 26 for a few things: - The "modern" implementation of fake timers [1], for zulip#4165. - The `selectProjects` CLI argument [2], for testing Android codepaths in a nice way (later in this series). But we've *also* realized that we don't really want to jettison `jest-expo` again, as we've done a few times as we learn new things: 62621ef jest: Add `jest-expo` preset, to be used in the next commit. 347aa96 jest: Back out `jest-expo` preset, for now. c4fca9d jest: Add and use `jest-expo` preset, again. In particular, we really want to use the `jest-expo/ios` and `jest-expo/android` presets (which we'll do later in this series). This is a hack because the `resolutions.jest-expo/jest` range `^26.4.1` is incompatible with the original range that `jest-expo` declares; that's `^25.2.0`. We believe this should get us a warning from Yarn; according to a doc [3], "You will receive a warning if your resolution version or range is not compatible with the original version range." I haven't seen a warning like that, even after removing node_modules and running `yarn`; this seems like a Yarn bug. I've opened expo/expo#12735 to bump Jest in the `jest-expo` project; this hack should hopefully be fine while we wait for that. [0] zulip#4700 (comment) [1] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2600 [2] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2610 [3] https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-tips-tricks

As Greg explains; see zulip#4700 (comment).

As Greg suggests at zulip#4700 (comment). These made sense before `tools/test` existed, and they were good shortcuts for complicated commands. Now, even if they still work, they're just obstacles in the way of getting familiar with `tools/test` and all its features.

In my local test runs, at least, I noticed that none of the tests in the "Parse" describe block were getting run. That's not good! At least they pass after this commit that fixes that bug, though. When our code in the "Parse" describe block started running, the object at `stringified` was empty, so the `.forEach` loop was iterating through...an empty array. Our mistake was assuming that the code that built up that object would be finished running in time. It wasn't finished in time, because that code was inside a test block of its own. Jest tests generally run concurrently, so we can't generally expect one test to be run before another, even if we define the test blocks in a particular order.

As Greg points out [1], this isn't a best fit for snapshot tests. A blog post [2] linked from a Jest doc says, """ The first thing that became clear to me while using snapshot testing is that they’re not for everything. They are optimized for a different case than normal assertion-based tests. Classic assertion based tests are perfect for testing clearly defined behavior that is expected to remain relatively stable. Snapshot tests are great for testing less clearly defined behavior that may change often. """ Which we think is basically right. [1] zulip#4348 (comment) [2] https://benmccormick.org/2016/09/19/testing-with-jest-snapshots-first-impressions/

We've had Jest 26 as a direct dependency for a while (since fb23341), so it's surprising that the `jest` command hasn't been using it. As Greg points out [0], that turns out to be because `jest-expo` apparently provides its own wrapper for the `jest` command, using the Jest that `jest-expo` depends on. That's Jest 25, even in `jest-expo`'s latest. And Yarn ends up giving us that wrapper. We know we'll soon want to use Jest 26 for a few things: - The "modern" implementation of fake timers [1], for zulip#4165. - The `selectProjects` CLI argument [2], for testing Android codepaths in a nice way (later in this series). But we've *also* realized that we don't really want to jettison `jest-expo` again, as we've done a few times as we learn new things: 62621ef jest: Add `jest-expo` preset, to be used in the next commit. 347aa96 jest: Back out `jest-expo` preset, for now. c4fca9d jest: Add and use `jest-expo` preset, again. In particular, we really want to use the `jest-expo/ios` and `jest-expo/android` presets (which we'll do later in this series). This is a hack because the `resolutions.jest-expo/jest` range `^26.4.1` is incompatible with the original range that `jest-expo` declares; that's `^25.2.0`. We believe this should get us a warning from Yarn; according to a doc [3], "You will receive a warning if your resolution version or range is not compatible with the original version range." I haven't seen a warning like that, even after removing node_modules and running `yarn`; this seems like a Yarn bug. I've opened expo/expo#12735 to bump Jest in the `jest-expo` project; this hack should hopefully be fine while we wait for that. [0] zulip#4700 (comment) [1] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2600 [2] https://github.com/facebook/jest/blob/master/CHANGELOG.md#2610 [3] https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-tips-tricks

When we've used the `jest-expo` preset or the `react-native` preset, the tests get run "in the standard React Native environment (iOS)" [1]. The key piece of this is that they run with `Platform.OS` mocked to 'ios'. We're about to add another "project" to test Android codepaths with `jest-expo/android`. First, though, swap out the old `jest-expo` preset for this more explicit one so we don't have to think about three presets. [1] https://github.com/expo/expo/blob/master/packages/jest-expo/README.md#platforms

This means that our Jest tests will take ~twice as long as they took before. We'll soon add an option to `tools/test` to run the Jest tests for just one platform, which we can use locally to speed things up, just without getting as much coverage as we'd otherwise get. See discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60jest-expo.2Fios.60.20and.20.60jest-expo.2Fandroid.60/near/1169139.

We're about to add a `--platform` option, and we'd like it to determine the behavior of some test suites, including this one. So, give this an accurate but not platform-specific name. (That's a cleaner way to do it than having the new option alter the list of suites to run; such an interface would conflict with the current interface that allows the runner to list the suites they want to run.)

As Greg explains; see zulip#4700 (comment).

Using the workaround from the big warning we just put on `set -e` at the top.

As Greg suggests at zulip#4700 (comment). These made sense before `tools/test` existed, and they were good shortcuts for complicated commands. Now, even if they still work, they're just obstacles in the way of getting familiar with `tools/test` and all its features.

Soon, this option will grow to include the meaning "run tests for both platforms", when we add a `--platform` option, coming up.

And expand `--all`, which runs in CI and at releases, to include the behavior of `--platform both`. Leave the current behavior as the default, with the name "sloppy", to not slow down local testing. Greg points out [1], """ From looking at a couple of recent CI builds, it looks like running Jest both ways doubles the Jest time, as you'd expect; it goes from about 60s to about 120s. Our CI times are around 8-9 minutes now, so that adds one minute -- not awesome, but I think worth it for getting coverage of this sort of code. """ [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60jest-expo.2Fios.60.20and.20.60jest-expo.2Fandroid.60/near/1169341

And promote a `global-require` ESLint ignore to a file-global ignore, rather than figuring out how to get the `$FlowIgnore` and the line-specific ESLint ignore to both work.

chrisbobbe · 2021-05-10T19:40:51Z

Thanks for the reviews! @gnprice, I've decided to merge, having timed out on waiting for discussion and some changes you mentioned you'd like to make, on last week's call. See discussion here where I gave some heads-up about this plan. 🙂

I think the risk of doing this is unknown, but most likely low: the last review (#4700 (review)) only requested tiny changes, and I made those. My sense is that the more substantive things Greg has in mind are mostly follow-up changes that don't require things to change inside this branch; I'll file an issue for those changes to be described and then made (edit: #4742). E.g., I guess it'd be ideal to have the best interface from the beginning, but addressing something like #4700 (comment) can very likely be done as a followup; things aren't unusably broken without that.

Now, we can

proceed with Add tests for ZulipAsyncStorage.getItem and .setItem. #4709 and Structure redux-persist code toward doing writes in batches with AsyncStorage.multiSet. #4694 (top priority)
make better headway on a debugging point at unreads: Don't add messages with "read" flag to unreads. #4710 (comment)
remove a step in the jest-expo uninstall/reinstall dance in Initial fetch timeout (with working tests) #4193 (88d6f50 in the current revision)

gnprice · 2021-05-24T19:34:42Z

Thanks @chrisbobbe! Sorry I dropped this for a bit. I actually thought I'd merged it an hour or two before you eventually did (i.e., late that morning here); must be that that failed with a "there are upstream changes, you need to rebase first" and I didn't notice.

Here's the version I thought I'd pushed:
a3e4c0f replaceRevive tests: Fix bug where "Parse" describe block did nothing.
5a90e13 replaceRevive tests: Stop using snapshot tests.
cf6f725 deps: Use Jest 26, the latest, with a resolutions hack.
2a53ad6 jest: Use jest-expo/ios preset.
80a68be jest: Expand coverage to include Android-only codepaths.
7f93fc2 jestSetup [nfc]: Start type-checking.
fe68290 jestSetup [nfc]: Remove blank line between third-party imports.
99ea911 tools/test [nfc]: Write down a big gotcha with set -e.
6623263 tools/test [nfc]: Explain the set -e thing a bit more broadly.
9009158 yarn scripts: Remove several aliases for simple commands.
4d628c7 tools/test: Rename --full to --all.
716f8a6 tools/test: Rename android suite to native.
6efa08a tools/test [nfc]: Bring Android-native test code to its own function.
0ee8fd6 tools/test: Add --platform option.
03b7992 tools/test [nfc]: Reorganize usage message.
2d632b5 tools/test [nfc]: Simplify description of default list of suites.
b43928a docs/testing: Explain more on Jest platform switch; shorten in howto.

I'll go ahead and push the additional commits that were in that version -- all comment- and doc-only changes. Other than those, the differences were just

I had the changes in an order that felt a bit more logical, e.g. grouping the Jest-setup changes together.

One clarified summary line:

-    tools/test [nfc]: Write down a big gotcha.
+    tools/test [nfc]: Write down a big gotcha with `set -e`.

I.e. the gotcha is in set -e, not in using tools/test.

So indeed, merging this version as it was was just fine. 🙂

Then after those I have some draft changes which split native back up into android plus a future/placeholder ios (along the lines of #4700 (comment) ), and then go on to experiment with some refactorings to help manage the should-skip logic for each of these suites while communicating clearly to the user what's happening. I'll see about finishing those up into a PR.

gnprice · 2021-05-24T19:42:54Z

OK, pushed those additional commits:
f6985b0 tools/test [nfc]: Explain the set -e thing a bit more broadly.
8e4c096 tools/test [nfc]: Reorganize usage message.
15cd56b tools/test [nfc]: Simplify description of default list of suites.
47c83c4 docs/testing: Explain more on Jest platform switch; shorten in howto.

chrisbobbe · 2021-05-24T20:18:22Z

Great, thanks!

This will give us more coverage of android when we're running tools/test in sloppy mode (which is the default). This does make things somewhat flaky, but on the whole it makes us more likely to notice failures that only occur on one platform sooner. See prior discussion: zulip#4700 (comment)

This will give us more coverage of android when we're running tools/test in sloppy mode (which is the default). This does make things somewhat flaky, but on the whole it makes us more likely to notice failures that only occur on one platform sooner. See prior discussion: #4700 (comment)

chrisbobbe requested review from gnprice and WesleyAC April 27, 2021 15:58

chrisbobbe mentioned this pull request Apr 27, 2021

Structure redux-persist code toward doing writes in batches with AsyncStorage.multiSet. #4694

Merged

gnprice reviewed Apr 27, 2021

View reviewed changes

gnprice added the a-tools label Apr 27, 2021

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 29, 2021

tools/test [nfc]: Write down a big gotcha.

ee01be9

As Greg explains; see zulip#4700 (comment).

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 29, 2021

tools/test: Add --all option to alias --all-files --platforms both.

6ba5e82

And promote it in the release docs and run it in CI. As Greg suggests: zulip#4700 (comment).

chrisbobbe force-pushed the pr-android-jest-coverage branch from 4cb5033 to b17a4f3 Compare April 29, 2021 00:29

WesleyAC reviewed Apr 29, 2021

View reviewed changes

gnprice reviewed Apr 29, 2021

View reviewed changes

tools/test Outdated Show resolved Hide resolved

docs/howto/testing.md Outdated Show resolved Hide resolved

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 30, 2021

tools/test [nfc]: Write down a big gotcha.

d3a4074

As Greg explains; see zulip#4700 (comment).

chrisbobbe force-pushed the pr-android-jest-coverage branch from b17a4f3 to 64e9db6 Compare April 30, 2021 21:54

chrisbobbe mentioned this pull request May 1, 2021

Add tests for ZulipAsyncStorage.getItem and .setItem. #4709

Merged

gnprice reviewed May 3, 2021

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 4, 2021

tools/test [nfc]: Write down a big gotcha.

51300fd

As Greg explains; see zulip#4700 (comment).

chrisbobbe force-pushed the pr-android-jest-coverage branch from 64e9db6 to 0250567 Compare May 4, 2021 23:26

chrisbobbe added the P1 high-priority label May 5, 2021

gnprice pushed a commit to gnprice/zulip-mobile that referenced this pull request May 6, 2021

tools/test [nfc]: Write down a big gotcha with set -e.

99ea911

As Greg explains; see zulip#4700 (comment).

chrisbobbe mentioned this pull request May 7, 2021

unreads: Don't add messages with "read" flag to unreads. #4710

Merged

chrisbobbe added 13 commits May 10, 2021 15:12

tools/test [nfc]: Write down a big gotcha.

ae03279

As Greg explains; see zulip#4700 (comment).

tools/test [nfc]: Bring Android-native test code to its own function.

fa8cd5d

Using the workaround from the big warning we just put on `set -e` at the top.

tools/test: Rename --full to --all.

673db66

Soon, this option will grow to include the meaning "run tests for both platforms", when we add a `--platform` option, coming up.

jestSetup [nfc]: Start type-checking.

3f2bba3

And promote a `global-require` ESLint ignore to a file-global ignore, rather than figuring out how to get the `$FlowIgnore` and the line-specific ESLint ignore to both work.

jestSetup [nfc]: Remove blank line between third-party imports.

5fd977a

chrisbobbe force-pushed the pr-android-jest-coverage branch from 0250567 to 5fd977a Compare May 10, 2021 19:18

chrisbobbe merged commit 5fd977a into zulip:master May 10, 2021

chrisbobbe mentioned this pull request May 10, 2021

Follow-up from #4700, "Have Jest cover Android-only codepaths." #4742

Open

chrisbobbe mentioned this pull request Jun 18, 2021

test: Pick platform randomly in "sloppy" mode. #4795

Closed

chrisbobbe deleted the pr-android-jest-coverage branch November 4, 2021 21:54

		@@ -1,4 +1,21 @@
		#!/usr/bin/env bash

		# Careful! This doesn't do everything you'd think it does. In fact, we

		compress: async (input: string) => `${header}${input}`,
		decompress: async (input: string) => input.replace(header, ''),

Have Jest cover Android-only codepaths. #4700

Have Jest cover Android-only codepaths. #4700

Conversation

chrisbobbe commented Apr 27, 2021 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 27, 2021 • edited Loading

chrisbobbe commented Apr 29, 2021

WesleyAC 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

chrisbobbe Apr 29, 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

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 30, 2021 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented May 4, 2021 • edited Loading

chrisbobbe commented May 10, 2021 • edited Loading

gnprice commented May 24, 2021

gnprice commented May 24, 2021

chrisbobbe commented May 24, 2021

chrisbobbe commented Apr 27, 2021 •

edited

Loading

chrisbobbe Apr 27, 2021 •

edited

Loading

chrisbobbe Apr 27, 2021 •

edited

Loading

chrisbobbe Apr 28, 2021 •

edited

Loading

chrisbobbe commented Apr 27, 2021 •

edited

Loading

chrisbobbe Apr 29, 2021 •

edited

Loading

chrisbobbe commented Apr 30, 2021 •

edited

Loading

chrisbobbe commented May 4, 2021 •

edited

Loading

chrisbobbe commented May 10, 2021 •

edited

Loading