[Tests] Yeoman tests are flaky (specifically generator-test and generator-android-test)

[ide]

These tests sometimes fail on Travis and on my local machine as well. My suspicion is that it has to do with how the tests are scheduled since the generator tests write to a temporary directory on disk, and the Yeoman APIs are global so maybe there isn't good isolation between tests (there is one assert.file function that somehow knows what the current temp dir is).

Perhaps we should disable these tests for now, or run the local-cli tests separately from the JS tests and mark them as "allowed to fail" in Travis.

[ide]

I also noticed the Podfile test failed on my local machine when running the jest tests in-band. I believe we should look into mocking "fs" if that's possible.

[mkonicek]

Thanks for reporting!

[ide]

Sure thing. A couple other things I observed were:

• The waitsFor block in the tests seems to be completing successfully. I didn't see it time out, and I also didn't see "error" events from the Yeoman RunContext (we get only "end" events).
• There is some code to silence some noisy console.log calls in the tests. I copied it over to generator-android-test.js and it fixed the noisiness but the test still sometimes failed.

[mkonicek]

Example of a failed test run: https://travis-ci.org/facebook/react-native/jobs/81832471

[mkonicek]

Looks like we're using the Yeoman test helpers correctly: http://yeoman.io/authoring/testing.html
Maybe we're not using waitsFor correctly, still reading the code.

[mkonicek]

The tests not being isolated properly sounds like a reasonable explanation as well. Does npm test run tests in parallel?

[mkonicek]

It looks like the tests run sequentially when I run them locally.

[mkonicek]

One way to debug this would be to add logging to the tests and check output on Travis.

[ide]

Jest will spawn child processes for each test file, but I believe the specs within each file run sequentially.

One way to debug this would be to add logging to the tests and check output on Travis.

I looked into that and saw that beforeEach -> spec 1 -> beforeEach -> spec 2 -> etc... so it seemed like the specs would run sequentially. The fact that the tests are flaky made me think it had to do with scheduling though.

[ide]

Maybe we're not using waitsFor correctly, still reading the code.

I think waitsFor is fine and the timeout message is never printed. We probably don't need the runAllTicks/Timers calls though because auto-mocking is disabled.

[ide]

Note: after the tests are more reliable we should delete allowed_failures from .travis.yml: https://github.com/facebook/react-native/blob/master/.travis.yml#L74

[mkonicek]

👍 Thanks for the "fix"! :)

Asked whether Yeoman tests can be run in parallel here: http://stackoverflow.com/questions/32751180/can-yeoman-tests-be-run-in-parallel

[mkonicek]

Turns out the Yeoman tests rely on changing the current working directory. Therefore they can't be run in parallel: http://stackoverflow.com/questions/32751180/can-yeoman-tests-be-run-in-parallel/32751527

[ide]

Nice find :) We could manually mock out fs and process.chdir, which is called here: https://github.com/yeoman/generator/blob/master/lib/test/helpers.js#L87. I do worry about the chdir call having side effects that we have not anticipated.

[mkonicek]

Yes just saw that line too and was thinking of how to avoid relying on the CWD. Since run creates a temp folder, we could simply replace all our asserts to check that test folder, instead of using Yeoman's assert. Although I think that a simple in-memory fs would be nicer than having to actually write to disk and then read from it.

[mkonicek]

Or we could just tell Jest to run the only CLI tests sequentially and remove allowed_failures.

[ide]

I'd prefer we try to get the mock fs + process.chdir working, and if that fails then run the CLI tests sequentially because there is a measurable perf improvement with the parallelism on my machine (2~3x I think) .

For in-memory fs, webpack has one that it has used for quite awhile: https://www.npmjs.com/package/memory-fs

[mkonicek]

Cool, can give it a try tomorrow unless you're faster while I'm asleep :)

[ide]

OK. I probably won't get around to it but am happy to review if you'd like an extra pair of eyes on it.

[mkonicek]

kk

[cpojer]

Yeah, we should aim at mocking the things that are clashing and make sure each test can be run safely in isolation. Let me know if you need any help @mkonicek.

[foghina]

Hmm, how has this only now started to happen? Did something change in Jest? Did tests use to run sequentially? It's weird because these tests were stable for a long time, and the tests themselves haven't changed.

Perhaps an easier fix than mocking fs would be to simply change these tests to use different name arguments for the generator (e.g. TestAppIOS, TestAppAndroid, TestApp), this way they shouldn't overwrite each other in the temp dir. Thoughts?

[mkonicek]

Talked to @foghina, the right solution is to submit a PR to Yeoman. Mocking fs won't help, the key thing to mock would be os.chdir but Yeoman relies on it too much.
As a quick fix we'll put the tests in a single describe so they run sequentially.

[cpojer]

What about what @foghina said and using a different name for the generator? That seems smarter.

[ide]

#3000 (different names) seems to be working well. If we see more failures in the future let's revisit this issue then.