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

feat(config): add read initial options helper #13356

Merged
merged 24 commits into from Oct 3, 2022

Conversation

nicojs
Copy link
Contributor

@nicojs nicojs commented Oct 2, 2022

Summary

Add readInitialOptions function to the jest-config package. This helper function can be useful for consumers of the jest programmatic API. It exposes the functionality that jest uses to read config from different sources.

Test plan

I've added integration tests to the jest-config package. I couldn't add tests for a jest.config.mjs file, because it kept being read as a cjs file. I think this has to do with babel transforming.

To add a test for a jest.config.ts file, I had to enable --experimental-vm-modules to run jest on jest. If you see problems with this, I can also remove the jest.config.ts test altogether (or we can fix it somehow?)

I've tested it on the integration test suite in StrykerJS, and it works as expected.

Closes #13348

Add `readInitialOptions` function to the jest-config package. This is a helper function that can be useful for consumers of the jest programmatic api. It exposes the functionality that jest uses to read config from different sources.
@nicojs
Copy link
Contributor Author

nicojs commented Oct 2, 2022

@SimenB I think readInitialOptions is a better name than readRawOptions. Do you agree?

I've decided to make the API a bit more precise; instead of 5 positional arguments, we now have 2 of which the second argument is an "options" parameter, where you are forced to explicitly name each option you want to override.

@nicojs
Copy link
Contributor Author

nicojs commented Oct 2, 2022

I don't know why the circleci tests are failing 😟

@SimenB
Copy link
Member

SimenB commented Oct 2, 2022

I've added integration tests to the jest-config package.

Integration test should be in e2e/ directory. That way there's no need for experimental flags either

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting!

CHANGELOG.md Outdated Show resolved Hide resolved
packages/jest-config/src/index.ts Outdated Show resolved Hide resolved
packages/jest-config/src/index.ts Outdated Show resolved Hide resolved
* Indicates whether or not to ignore the error of jest finding multiple config files.
* (default is false)
*/
skipMultipleConfigError?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be an option? Seems one should always fail of that's the case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was already supported by readConfig. Since readConfig is using readInitialOptions internally I thought I would add this feature to readInitialOptions as well.

Removing this feature altogether might be a breaking change, right?

@nicojs
Copy link
Contributor Author

nicojs commented Oct 2, 2022

@SimenB thanks for the fast review!

With regards to --experimental-vm-modules, reading from a jest.config.ts file without it doesn't seem to work in e2e directory either. I've pushed it now so we can see the error in CI.

I get this weird Segmentation fault error right before the test process crashes:

nicojs@NicoJ01 ~/github/jest (feat/read-initial-options)$  cd /home/nicojs/github/jest ; /usr/bin/env 'NODE_OPTIONS=--require /home/nicojs/.vscode-server/bin/74b1f979648cc44d385a2286793c226e611f59e7/extensions/ms-vscode.js-debug/src/bootloader.bundle.js --inspect-publish-uid=http' 'VSCODE_INSPECTOR_OPTIONS={"inspectorIpc":"/tmp/node-cdp.943-83.sock","deferredMode":false,"waitForDebugger":"","execPath":"/home/nicojs/.nvm/versions/node/v16.15.0/bin/node","onlyEntrypoint":false,"autoAttachMode":"always","mandatePortTracking":true,"fileCallback":"/tmp/node-debug-callback-45176a3390f07d11"}' /home/nicojs/.nvm/versions/node/v16.15.0/bin/node ./node_modules/.bin/jest readInitialOptions.test 
Debugger attached.

 RUNS  e2e/__tests__/readInitialOptions.test.ts
Segmentation fault

UPDATE: I think I still had an error in my implementation. I get this clear error now:

● readInitialOptions › should read a jest.config.ts file

    Jest: Failed to parse the TypeScript config file /home/nicojs/github/jest/e2e/read-initial-options/ts-config/jest.config.ts
      Error: You need to run with a version of node that supports ES Modules in the VM API. See https://jestjs.io/docs/ecmascript-modules

      38 |     const configFile = resolveFixture('ts-config', 'jest.config.ts');
      39 |     const rootDir = resolveFixture('ts-config');
    > 40 |     const {config, configPath} = await readInitialOptions(configFile);
         |                                  ^
      41 |     expect(config).toEqual({jestConfig: 'jest.config.ts', rootDir});
      42 |     expect(configPath).toEqual(configFile);
      43 |   });

      at readConfigFileAndSetRootDir (packages/jest-config/build/readConfigFileAndSetRootDir.js:136:13)
      at Object.<anonymous> (e2e/__tests__/readInitialOptions.test.ts:40:34)

Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@mrazauskas
Copy link
Contributor

The e2e test for reading jest.config.ts lives here. Perhaps there is some clue?

Just an idea – would you consider using the pattern from the mentioned file for the e2e test in this PR? It works well with simple fixtures as in this case.

@nicojs
Copy link
Contributor Author

nicojs commented Oct 2, 2022

The difference between jest.config.ts.test.ts and these new e2e tests is that I'm using the readInitialConfig API directly, while the other tests are spawning a new jest instance. It probably has to do with that.

The problem lies in this line of code:

https://github.com/nicojs/jest/blob/c2dffe995af845ccc0bd4affc936d8013f90bf19/packages/jest-config/src/readConfigFileAndSetRootDir.ts#L112

image

I don't know why a import expression would yield such an error 🤷‍♀️

nicojs and others added 4 commits October 2, 2022 15:23
@mrazauskas
Copy link
Contributor

If I did not miss something, this e2e test is passing:

import * as path from 'path';
import execa = require('execa');
import {cleanup, writeFiles} from '../Utils';

const DIR = path.resolve(__dirname, '../read-initial-options');

beforeEach(() => cleanup(DIR));
afterAll(() => cleanup(DIR));

test('reads a jest.config.ts file', async () => {
  writeFiles(DIR, {
    'jest.config.ts': "export default {jestConfig: 'jest.config.ts'};",
    'package.json': '{}',
    'readConfig.js': `
      const {readInitialOptions} = require('jest-config');
      async function readConfig() {
        console.log(await readInitialOptions());
      }
      readConfig();
    `,
  });

  const {stderr, stdout} = await execa('node', ['readConfig.js'], {cwd: DIR});

  expect(stderr).toBe('');
  expect(stdout).toMatch("jestConfig: 'jest.config.ts'");
});

@nicojs
Copy link
Contributor Author

nicojs commented Oct 2, 2022

Yeah, exactly. This is what I was thinking later as well. That approach will also work for a jest.config.mjs file I think. Probably the root cause is jest running tests in a node VM (at least, that is my understanding now).

I will update this PR tomorrow morning.

@nicojs
Copy link
Contributor Author

nicojs commented Oct 3, 2022

I've fixed my test 🎉

Don't know why some pipelines are failing 😢

Comment on lines 52 to 55
// Read by proxy, because we're running in a VM and are not allowed to `import` directly
const {stdout} = await execa('node', ['../readOptions.js'], {
cwd: rootDir,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding this approach should be used in all test in this file. It is based on real world implementation and that is how I like e2e tests to be written.

In contrary, importing readInitialOptions to a test file and mocking process.cwd still feels like a unit test. Of course, that just an approach or opinion. The same thing is tested either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, using execa is much better. Fixed with the latest push

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good!

// Otherwise just try to find config in the current rootDir.
configPath = resolveConfigPath(
const {config: initialOptions, configPath} = await readInitialOptions(
argv?.config,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
argv?.config,
argv.config,

Copy link
Contributor Author

@nicojs nicojs Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails the readConfig test. I assumed you would want to keep supporting it.

See packages/jest-config/src/tests/readConfig.test.ts

test('readConfig() throws when an object is passed without a file path', async () => {
  await expect(
    readConfig(
      // @ts-expect-error
      null /* argv */,
      {} /* packageRootOrConfig */,
      false /* skipArgvConfigOption */,
      null /* parentConfigPath */,
    ),
  ).rejects.toThrowError(
    'Jest: Cannot use configuration as an object without a file path',
  );
});

Copy link
Member

@SimenB SimenB Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a bug in the test - argv is always called according to the types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, updated the tests and removed this ?

packages/jest-config/src/index.ts Outdated Show resolved Hide resolved
e2e/__tests__/readInitialOptions.test.ts Outdated Show resolved Hide resolved
e2e/__tests__/readInitialOptions.test.ts Outdated Show resolved Hide resolved
nicojs and others added 2 commits October 3, 2022 10:22
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Comment on lines 298 to 299
// A string passed to `--config`, which is either a direct path to the config
// or a path to directory containing `package.json`, `jest.config.js` or `jest.config.ts`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be moved two lines down. Or?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! Fixed

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@SimenB SimenB merged commit d78baab into jestjs:main Oct 3, 2022
@nicojs nicojs deleted the feat/read-initial-options branch October 3, 2022 16:17
@SimenB
Copy link
Member

SimenB commented Oct 14, 2022

https://github.com/facebook/jest/releases/tag/v29.2.0

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Expose readRawConfig from jest-config
4 participants