Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have Jest cover Android-only codepaths. #4700

Merged
merged 13 commits into from
May 10, 2021

Commits on May 10, 2021

  1. replaceRevive tests: Fix bug where "Parse" describe block did nothing.

    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.
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    63602a6 View commit details
    Browse the repository at this point in the history
  2. replaceRevive tests: Stop using snapshot tests.

    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/
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    698b4de View commit details
    Browse the repository at this point in the history
  3. deps: Use Jest 26, the latest, with a resolutions hack.

    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
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    f1b59fe View commit details
    Browse the repository at this point in the history
  4. jest: Use jest-expo/ios preset.

    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
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    c431836 View commit details
    Browse the repository at this point in the history
  5. jest: Expand coverage to include Android-only codepaths.

    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.
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    85a2cbf View commit details
    Browse the repository at this point in the history
  6. tools/test: Rename android suite to native.

    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.)
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    d0a8d9e View commit details
    Browse the repository at this point in the history
  7. tools/test [nfc]: Write down a big gotcha.

    As Greg explains; see
      zulip#4700 (comment).
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    ae03279 View commit details
    Browse the repository at this point in the history
  8. tools/test [nfc]: Bring Android-native test code to its own function.

    Using the workaround from the big warning we just put on `set -e` at
    the top.
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    fa8cd5d View commit details
    Browse the repository at this point in the history
  9. yarn scripts: Remove several aliases for simple commands.

    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 committed May 10, 2021
    Configuration menu
    Copy the full SHA
    8bd4fa1 View commit details
    Browse the repository at this point in the history
  10. tools/test: Rename --full to --all.

    Soon, this option will grow to include the meaning "run tests for
    both platforms", when we add a `--platform` option, coming up.
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    673db66 View commit details
    Browse the repository at this point in the history
  11. tools/test: Add --platform option.

    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
    chrisbobbe committed May 10, 2021
    Configuration menu
    Copy the full SHA
    d9d8af0 View commit details
    Browse the repository at this point in the history
  12. jestSetup [nfc]: Start type-checking.

    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 committed May 10, 2021
    Configuration menu
    Copy the full SHA
    3f2bba3 View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    5fd977a View commit details
    Browse the repository at this point in the history