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

Avoid test collisions by using different directories in runInTempDirectory #902

Merged
merged 2 commits into from
May 9, 2019

Conversation

CvX
Copy link
Contributor

@CvX CvX commented May 8, 2019

The tests were randomly failing for me:

 FAIL  __tests__/cli.test.js (7.722s)
   cli  build  creates compiled CSS file

    ENOENT: no such file or directory, open 'output.css'

      128 |  */
      129 | export function writeFile(path, content) {
    > 130 |   ensureFileSync(path)
          |   ^
      131 |
      132 |   return outputFileSync(path, content)
      133 | }

      at createFileSync (node_modules/fs-extra/lib/ensure/file.js:43:6)
      at Object.writeFile (src/cli/utils.js:130:3)
      at writeFile (src/cli/commands/build.js:86:11)

I believe it's a timing issue with the tests being run in parallel.
This PR fixes that by using a different temporary directory for each test.

@adamwathan
Copy link
Member

This has been driving me mental for months because it's so sporadic, thanks for taking the time to dig in to it. Looking at your implementation made me wonder, "does Jest provide a unique ID for each process?" and it turns out it does!

jestjs/jest#5494

I think the timestamp solution is probably robust enough but I wonder if using this ID would be a better idea?

@CvX
Copy link
Contributor Author

CvX commented May 9, 2019

Nice find! There isn't much difference, but I like the use of JEST_WORKER_ID because lets people reading the code know that this part has to do with jest workers. 🙂 Updated!

@adamwathan adamwathan merged commit a7aedde into tailwindlabs:next May 9, 2019
@adamwathan
Copy link
Member

I'm gonna be so happy if I never see that stupid false test failure ever again 😅 Thanks so much!

@adamwathan
Copy link
Member

Goddammit it's still happening for me. I wish I could come up with a bullet proof way to reproduce but it basically just happens around 50% of the time I run the full test suite after making any code change 😫

The weird thing is that the symptom I actually see is that a tailwind.config.js file gets created in the project root when it's supposed to be created in a temp directory, so it's like the CWD gets messed up for one of the processes or something.

This test some writes the file to the wrong directory sometimes:

test('tailwind.config.js is picked up by default', () => {
  return inTempDirectory(() => {
    fs.writeFileSync(
      path.resolve(defaultConfigFile),
      `module.exports = {
        theme: {
          screens: {
            mobile: '400px',
          },
        },
      }`
    )

    return postcss([tailwind])
      .process(
        `
          @responsive {
            .foo {
              color: blue;
            }
          }
        `,
        { from: undefined }
      )
      .then(result => {
        expect(result.css).toMatchCss(`
          .foo {
            color: blue;
          }
          @media (min-width: 400px) {
            .mobile\\:foo {
              color: blue;
            }
          }
        `)
      })
  })
})

All this parallel test worker shit is really unfriendly to any sort of integration testing :/

@CvX
Copy link
Contributor Author

CvX commented May 9, 2019

We could always run tests sequentially:

// package.json
-    "test": "jest && eslint ."
+    "test": "jest --runInBand && eslint ."

There doesn't seem to be much difference duration wise:

# Parallel
Ran all test suites.
✨  Done in 9.26s.

# Sequential
Ran all test suites.
✨  Done in 10.17s.

Though it would be nice to find an actual solution to these problems. But it's difficult to debug it further for me, since I don't have failures anymore. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants