firsttris / vscode-jest-runner

Simple way to run or debug one or more tests from context menu, codelens or command plalette
https://marketplace.visualstudio.com/items?itemName=firsttris.vscode-jest-runner
MIT License
264 stars 124 forks source link

jestRunnerConfig.ts does not support package.json #220

Open bibobibo opened 2 years ago

bibobibo commented 2 years ago

Jest support config from package.json (https://jestjs.io/docs/configuration), while the current jestRunnerConfig.ts doesn't support it. https://github.com/firsttris/vscode-jest-runner/blob/2401fa16e28d4ada4b4c3861706c84ea177c2655/src/jestRunnerConfig.ts#L109. Are you happy to support config from package.json as well? I am happy to make the change.

bibobibo commented 2 years ago

here is the pr

dkurucz commented 2 years ago

absolutely need this, any idea on when this might go in?

ehaynes99 commented 2 years ago

Can this be merged? It's all ready to go...

ehaynes99 commented 2 years ago

Hrmm, this doesn't work in a monorepo. :( It relies on currentPackagePath, which searches for the directory where both a package.json and a node_modules/jest dir exist. While this is correct for finding the jest executable, it's not necessarily the proper place to check for configuration. I'll try to take a stab at a PR this evening.

Out of curiosity, wouldn't this whole thing be less complicated if it simply did a cd into the directory containing the test and ran npx jest? That would find the correct binary, and jest would resolve the config on its own...

  private get currentPackagePath() {
    let currentFolderPath: string = path.dirname(vscode.window.activeTextEditor.document.fileName);
    do {
      // Try to find where jest is installed relatively to the current opened file.
      // Do not assume that jest is always installed at the root of the opened project, this is not the case
      // such as in multi-module projects.
      const pkg = path.join(currentFolderPath, 'package.json');
      const jest = path.join(currentFolderPath, 'node_modules', 'jest');
      if (fs.existsSync(pkg) && fs.existsSync(jest)) {
        return currentFolderPath;
      }
      currentFolderPath = path.join(currentFolderPath, '..');
    } while (currentFolderPath !== this.currentWorkspaceFolderPath);

    return '';
  }

I'll take a stab at a PR this evening

firsttris commented 2 years ago

should i revert it ?

ehaynes99 commented 2 years ago

No, this should work for standard projects, so still improved from before the merge. findConfigPath just needs separate search logic to allow for nested package.json files that aren't necessarily peers of node_modules/jest so that it will work for monorepos.

firsttris commented 2 years ago

had to revert this change cause it also broke mono-repos with dedicated jest-config, see here https://github.com/firsttris/vscode-jest-runner/issues/268

ehaynes99 commented 2 years ago

How about #270

This changes JestRunnerConfig#findConfigPath to check for a jest config in any package.json during the same upwards traversal as it uses for the other file types.

However, the more I look at this, do you really need to find the config file at all? Jest should resolve this automatically from either the directory containing the test, or the directory with the nearest package.json (or technically, any directory in between), so you would only need to specify it if it's overridden in the settings.

E.g.

.  <---- works if cwd here
├── package.json
└── test
    └── some
        └── path
            └── to  <---- or if cwd here
                └── some.test.ts

Or in a monorepo:

.  <---- Does NOT work here
├── package.json
└── packages
    └── some-package  <---- works if cwd here
        ├── package.json
        └── test
            └── some
                └── path
                    └── to  <---- or if cwd here
                        └── some.test.ts

If you do definitely want to locate it, though, there are other types jest supports: https://jestjs.io/docs/configuration

The file will be discovered automatically, if it is named jest.config.js|ts|mjs|cjs|json.

firsttris commented 2 years ago

I was also thinking about this, why are we specifying it at all. It should be resolved automatically.

Maybe it was something like: if someone opens the monorepo with the root path in vscode and then clicks "Run Jest" in some Package. We would need to specify it, otherwise it resolved to the root jest-config?

But I'm not sure anymore what was the reason, or who it even implement and why ..

ehaynes99 commented 2 years ago

Heh, fair enough. Anyhow, I tested my PR in a regular repo and a monorepo in a handful of scenarios with npm, and it seems ok. I have not tried it with yarn or pnpm, but since this has nothing to do with the executable, I don't think it would make a difference.

firsttris commented 2 years ago

Thx 👍 mate. Seems you have a good overview of the project. Can I add you as Collaborator?