perf: wrap tests in check to make sure app is ready when they run

[kieckhafer]

Resolves #5315
Impact: minor
Type: performance

Issue

CI test-app tests intermittently fail with a timeout, but usually pass on a second or third run.

Solution

Wrap each test in Reaction.onAppStartupComplete(() => { /* tests */ }) to make sure app is ready before tests run.

Breaking changes

None

Testing

1. run tests a multiple times
2. see that they pass every time

[kieckhafer]

@aldeed @focusaurus not sure the best way to test this, since it was an intermittent thing. I ran locally and it passed a couple times, and it passed the first time when pushing it to CI, so that's a good sign at least.

[focusaurus]

I'm worried putting the describe calls inside the event handler function might make mocha unaware that the tests even exist. Are you sure these will run every time? I'd be inclined to provide a helper function we can wrap just around the it handler like pseudocode:

import { appIsReady } from "some-test-utils-module";
describe("used outside of a connection", () => {
  ConnectionDataStore.set("key", "val");
  it(
    "sets/gets cached data",
    appIsReady(() => {
      ConnectionDataStore.set("key", "val");

      expect(ConnectionDataStore.get("key")).to.equal("val");
      expect(ConnectionDataStore.get("key")).to.equal("val");
    })
  );
});

Or use appIsReady around the describe handler function if the code in there needs to wait as well.

[focusaurus]

Could we also do this in a before() handler?

[kieckhafer]

@focusaurus most of the tests were already wrapped in this Reaction.onAppStartupComplete(() => {}), I only updated about 1/3rd of them so yes, if they've been working up until now, then I don't think this will change that.

[aldeed]

@focusaurus You are correct. Looking at the logging, it appears that tests passed because no tests were found. I remember thinking this might be happening at some point but forgot to check it. Sorry!

I like the idea of doing a before handler instead, and calling done() within Reaction.onAppStartupComplete. Something like

before((done) => {
  Reaction.onAppStartupComplete(() => {
    Fixtures();
    done();
  });
});

Didn't try it so I'm not sure it works. If the file calls Fixtures, that needs to happen after startup is complete. That was the original reason I wrapped some of them.

@kieckhafer the output when the tests run should say "N passed", where N is the number of it calls we have in the app-test files. On CI now it says "0 passed" so it's not seeing any of them.

[focusaurus]

OK I think that makes more sense. Already have lots of nesting due to the mocha API. I think this type of thing is a poster child for why before() exists.

[kieckhafer]

@aldeed does the before need to be done inside each describe? Or once per file?

[focusaurus]

I think you can have a top-level (not nested) before once per file before any describe calls and that should work.

[spencern]

@kieckhafer have you had a chance to look at this recently?

[kieckhafer]

@spencern been caught up with some other time-sensitive work this morning, will circle back once that is finished up.

[kieckhafer]

Made all the updates mentioned here, wrapping it all in a before and the issue still persists.

[focusaurus]

Bummer. Not sure just from code inspection what the issue is then.

[aldeed]

I will check out this branch and spend a couple minutes seeing if anything I can think of works.

[aldeed]

@kieckhafer @focusaurus I pushed a commit that has all of the Meteor app tests being reliably detected and run. I couldn't get many of the tests to be found, and then I noticed that they were all ones that were moved to /tests/meteor folder. So I decided to Google it, and it turns out that Meteor test runner ignores test files within /tests folder. Makes perfect sense, right? 🙄

Anyway I moved those to be in /imports and now they run.

The remaining problem is that since these haven't been running for awhile, there are at least 24 tests that need to be investigated and fixed. Most of these are probably issues with the mock data or APIs that changed after the tests stopped running. I don't have time to go through any more right now.

[aldeed]

@kieckhafer @focusaurus I think what's happening with the eslint check is that it's not omitting deleted / moved files from the list. Maybe there is a git diff option to omit those, or you could loop through and check for existence before passing to ESLint command.

[kieckhafer]

@aldeed @focusaurus added a check for deleted files: --diff-filter=d, that seemed to fix the issue.