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

Add runnerConfig option to configure runners #4278

Open
rogeliog opened this issue Aug 16, 2017 · 22 comments
Open

Add runnerConfig option to configure runners #4278

rogeliog opened this issue Aug 16, 2017 · 22 comments

Comments

@rogeliog
Copy link
Contributor

Do you want to request a feature or report a bug?

Enhancement

What is the current behavior?

There is no way of configuring a test runner

What is the expected behavior?

It would be nice to have a way to configure runners. For example: runnerConfig which could be any arbitrary object that is relevant for the runner configuration.

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.

This was referenced Aug 24, 2017
@rogeliog
Copy link
Contributor Author

rogeliog commented Sep 1, 2017

@cpojer @aaronabramov If we do decide to move forward with this, what do you think should be the interface here? runnerConfig: Object?

@aaronabramov
Copy link
Contributor

aaronabramov commented Sep 1, 2017

maybe something like what we do with reporters?

runner: 'some_runner.js'

// or

runner: ['some_runner.js', {hey: 'yo'}]

which translates to

// globalConfig.runner
{
  path: 'some_runner.js',
  options: {hey: 'yo'}
}

and then

const Runner = require(globalConfig.runner.path);
const runner = new Runner(globalConfig.runner.options);

@cpojer
Copy link
Member

cpojer commented Sep 1, 2017

I actually had something quite different in mind because ProjectConfig is mostly local to the runner. The ideal system would be to have a base ProjectConfig (with a runner option) that can include both the runner configuration as well as any custom configuration for that runner. ProjectConfig is then both the config for the project and the configuration for the runner.

The challenge here is how to make this type safe and how to make the configuration system ultimately configurable.

Most things in Jest would be typed with BaseProjectConfig with the minimal set of options that any runner (even for non-test use cases) would share, which is threaded to most places in Jest. Then there would be a type JestProjectConfig which is threaded to everything from jest-runner onwards to jest-jasmine etc. I think this change is quite involved, but the end-state sounds enticing to me. What do you think?

@aaronabramov
Copy link
Contributor

i'm a little worried about mixing Jest projectConfig with other custom configuration. I really don't like how it worked with global+project configs sharing the same JSON object, most of the bugs we discovered with MPR were super hard to debug and were caused by some values being in the wrong config. But i think if we have a separate typed projectConfig with a custom/possibly untyped projectConfig.runnerConfig it might work well

@cpojer
Copy link
Member

cpojer commented Sep 1, 2017

The GlobalConfig + ProjectConfig issues were a one time cost that the split brought with it, and I think now (as of Jest 21), the system is working quite smoothly. The thing that worries me the most is adding an uncontrollable amount of config and cli options (it's already a bit of a mess) and making ProjectConfig something that is theoretically pluggable would counter that, as people could build their own runners that cut down on the number of configuration options. Also, to build runners for things like prettier/eslint etc., all the test related configuration options don't really make any sense.

@ljharb
Copy link
Contributor

ljharb commented Jan 31, 2018

So is there still no way to pass custom options to a custom runner?

@borela
Copy link

borela commented Jul 18, 2018

Is anybody working on this?

@thymikee
Copy link
Collaborator

Nope, feel free to grab it :)

@borela
Copy link

borela commented Jul 18, 2018

I installed the dependencies and ran the tests, many of them break on windows, that needs to be solved first.

@SimenB
Copy link
Member

SimenB commented Jul 18, 2018

We have appveyor running as CI, so developing on windows should be fine... You'll need mercurial installed for a few tests, but besides that you should be good
What do you have issues with?

@borela
Copy link

borela commented Jul 18, 2018

It seams most tests are failing because somehow paths are being prepended with ../../../../../Alexandre/AppData/Local/Temp/.

 FAIL  packages/jest-resolve/src/__tests__/resolve.test.js
  ● resolveModule › is possible to resolve node modules by resolving their realpath

    Cannot find module '../../src/__mocks__/bar/node_modules/foo/index.js' from 'resolve.test.js'

      at Resolver.resolveModule (packages/jest-resolve/build/index.js:221:17)

 FAIL  e2e/__tests__/only_changed.test.js (45.124s)
  ● run only changed files

    expect(received).toMatch(expected)

    Expected value to match:
      /PASS __tests__(\/|\\)file1.test.js/
    Received:
      "PASS ../../../../../Alexandre/AppData/Local/Temp/jest_only_changed/__tests__/file1.test.js
      ✓ file1 (3ms)

    Test Suites: 1 passed, 1 total
    Tests:       1 passed, 1 total
    Snapshots:   0 total
    Time:        4.154s
    Ran all test suites related to changed files."

      43 |
      44 |   ({stderr} = runJest(DIR, ['-o', '--lastCommit']));
    > 45 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file1.test.js/);
         |                  ^
      46 |
      47 |   writeFiles(DIR, {
      48 |     '__tests__/file2.test.js': `require('../file2'); test('file2', () => {});`,

      at Object.<anonymous>.test (e2e/__tests__/only_changed.test.js:45:18)

  ● onlyChanged in config is overwritten by --all or testPathPattern

    expect(received).toMatch(expected)

    Expected value to match:
      /PASS __tests__(\/|\\)file1.test.js/
    Received:
      "PASS ../../../../../Alexandre/AppData/Local/Temp/jest_only_changed/__tests__/file1.test.js
      ✓ file1 (2ms)

    Test Suites: 1 passed, 1 total
    Tests:       1 passed, 1 total
    Snapshots:   0 total
    Time:        3.449s
    Ran all test suites related to changed files."

      143 |
      144 |   ({stderr} = runJest(DIR, ['--lastCommit']));
    > 145 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file1.test.js/);
          |                  ^
      146 |
      147 |   writeFiles(DIR, {
      148 |     '__tests__/file2.test.js': `require('../file2'); test('file2', () => {});`,

      at Object.<anonymous>.test (e2e/__tests__/only_changed.test.js:145:18)

  ● gets changed files for hg

    Error

      34 |       ERROR: ${result.error}
      35 |     `;
    > 36 |     throw new Error(message);
         |           ^
      37 |   }
      38 |
      39 |   return result;

      Error:
            ORIGINAL CMD: hg --config ui.username=jest_test init
            STDOUT:
            STDERR: 'hg' is not recognized as an internal or external command,
      operable program or batch file.
            STATUS: 1
            ERROR: undefined

      at run (e2e/Utils.js:36:11)
      at Object.<anonymous> (e2e/__tests__/only_changed.test.js:201:3)
      at step (e2e/__tests__/only_changed.test.js:26:191)
      at e2e/__tests__/only_changed.test.js:26:437
      at Object.<anonymous> (e2e/__tests__/only_changed.test.js:26:99)

  ● path on Windows is case-insensitive

    expect(received).toMatch(expected)

    Expected value to match:
      /PASS __tests__(\/|\\)file2.test.js/
    Received:
      "PASS ../../../../../../../Alexandre/AppData/Local/Temp/jest_only_changed/outer_dir/inner_dir/__tests__/file3.test.js
    PASS ../../../../../../../Alexandre/AppData/Local/Temp/jest_only_changed/outer_dir/inner_dir/__tests__/file2.test.js

    Test Suites: 2 passed, 2 total
    Tests:       2 passed, 2 total
    Snapshots:   0 total
    Time:        7.849s
    Ran all test suites related to changed files."

      267 |
      268 |   expect(stderr).not.toMatch(/PASS __tests__(\/|\\)file1.test.js/);
    > 269 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
          |                  ^
      270 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
      271 | });
      272 |

      at Object.<anonymous>.test (e2e/__tests__/only_changed.test.js:269:18)

 FAIL  e2e/__tests__/globals.test.js (35.212s)
  ● basic test constructs

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "basic test constructs 1".

    - Snapshot
    + Received

    @@ -1,6 +1,6 @@
    - "PASS __tests__/basic.test-constructs.test.js
    + "PASS ../../../../../Alexandre/AppData/Local/Temp/global-variables.test/__tests__/basic.test-constructs.test.js
        ✓ it
        ✓ test
        describe
          ✓ it
          ✓ test

      48 |
      49 |   const {summary, rest} = extractSummary(stderr);
    > 50 |   expect(rest).toMatchSnapshot();
         |                ^
      51 |   expect(summary).toMatchSnapshot();
      52 | });
      53 |

      at Object.<anonymous>.test (e2e/__tests__/globals.test.js:50:16)

  ● skips

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "skips 1".

    - Snapshot
    + Received

    @@ -1,6 +1,6 @@
    - "PASS __tests__/skips-constructs.test.js
    + "PASS ../../../../../Alexandre/AppData/Local/Temp/global-variables.test/__tests__/skips-constructs.test.js
        ✓ it
        ○ skipped 4 tests
        xdescribe
          ○ skipped 2 tests
        describe.skip

      78 |
      79 |   const {summary, rest} = extractSummary(stderr);
    > 80 |   expect(rest).toMatchSnapshot();
         |                ^
      81 |   expect(summary).toMatchSnapshot();
      82 |   expect(status).toBe(0);
      83 | });

      at Object.<anonymous>.test (e2e/__tests__/globals.test.js:80:16)

  ● only

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "only 1".

    - Snapshot
    + Received

    @@ -1,6 +1,6 @@
    - "PASS __tests__/only-constructs.test.js
    + "PASS ../../../../../Alexandre/AppData/Local/Temp/global-variables.test/__tests__/only-constructs.test.js
        ✓ test.only
        ✓ it.only
        ✓ fit
        ○ skipped 1 test
        fdescribe

      109 |
      110 |   const {summary, rest} = extractSummary(stderr);
    > 111 |   expect(rest).toMatchSnapshot();
          |                ^
      112 |   expect(summary).toMatchSnapshot();
      113 | });
      114 |

      at Object.<anonymous>.test (e2e/__tests__/globals.test.js:111:16)

  ● cannot test with no implementation

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "cannot test with no implementation 1".

    - Snapshot
    + Received

    @@ -1,6 +1,6 @@
    - "FAIL __tests__/only-constructs.test.js
    + "FAIL ../../../../../Alexandre/AppData/Local/Temp/global-variables.test/__tests__/only-constructs.test.js
        ● Test suite failed to run

          Missing second argument. It must be a callback function.

            1 |

      126 |
      127 |   const {summary} = extractSummary(stderr);
    > 128 |   expect(cleanStderr(stderr)).toMatchSnapshot();
          |                               ^
      129 |   expect(summary).toMatchSnapshot();
      130 | });
      131 |

      at Object.<anonymous>.test (e2e/__tests__/globals.test.js:128:31)

  ● skips with expand arg

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "skips with expand arg 1".

    - Snapshot
    + Received

    @@ -1,6 +1,6 @@
    - "PASS __tests__/skips-constructs.test.js
    + "PASS ../../../../../Alexandre/AppData/Local/Temp/global-variables.test/__tests__/skips-constructs.test.js
        ✓ it
        ○ xtest
        ○ xit
        ○ it.skip
        ○ test.skip

      157 |
      158 |   const {summary, rest} = extractSummary(stderr);
    > 159 |   expect(rest).toMatchSnapshot();
          |                ^
      160 |   expect(summary).toMatchSnapshot();
      161 | });
      162 |

      at Object.<anonymous>.test (e2e/__tests__/globals.test.js:159:16)

  ● only with expand arg

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "only with expand arg 1".

    - Snapshot
    + Received

    @@ -1,6 +1,6 @@
    - "PASS __tests__/only-constructs.test.js
    + "PASS ../../../../../Alexandre/AppData/Local/Temp/global-variables.test/__tests__/only-constructs.test.js
        ○ it
        ✓ test.only
        ✓ it.only
        ✓ fit
        fdescribe

      187 |
      188 |   const {summary, rest} = extractSummary(stderr);
    > 189 |   expect(rest).toMatchSnapshot();
          |                ^
      190 |   expect(summary).toMatchSnapshot();
      191 | });
      192 |

      at Object.<anonymous>.test (e2e/__tests__/globals.test.js:189:16)

@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Most things in Jest would be typed with BaseProjectConfig with the minimal set of options that any runner (even for non-test use cases) would share, which is threaded to most places in Jest. Then there would be a type JestProjectConfig which is threaded to everything from jest-runner onwards to jest-jasmine etc. I think this change is quite involved, but the end-state sounds enticing to me. What do you think?

That definitely sounds like an ideal solution, but I'm not sure how people would differentiate between project and global config. Do you imagine everything still being in a single flat configuration? Would it be possible at all to check for typos in configuration in normalize then, when we can't limit the keys present? I suppose we could load all global config, then load runners and either query them for schemas to pass config through for validation. I do think that typing out the config might be confusing for consumers though.

Something like this would feel like boilerplate I think:

{
  "jest": {
    // a bunch of global config,
    "runners": [
      // jest config
      {
        "module": "jest-runner",
        "testMatch": ["blabla"]
      },
      // eslint config
      {
        "module": "jest-runner-eslint",
        "include": ["blablabla"]
      }
    ]
  }
}

But any other option feels like it'd just be confusing, which is worse.


I do think @aaronabramov's first suggestion should be the way we go, at least for a first iteration. I do think to properly separate config we'll have to do a breaking change anyways, so why not design something properly at that point, but still solve the very real need of custom runner config with a simple tuple similar to reporters for now? That's also how we ended up passing config to watch plugins. Using cosmoconfig shouldn't be necessary.

@SimenB SimenB added this to the Jest 24 milestone Oct 16, 2018
@SimenB SimenB removed this from the Jest 24 milestone Jan 11, 2019
@elyobo
Copy link

elyobo commented Jun 7, 2019

Just tried to write a runner and was surprised to find that this doesn't exist. I guess ones like jest-runner-eslint just rely on eslint picking up its own config via the standard ways rather than getting it from Jest (e.g. package.json, .eslintrc.json, etc) and I guess that's the only way to go for now.

I don't entirely understand the interaction between the project config and global config, so can't really comment intelligently on @aaronabramov's original suggestion (it looks like it would only support it in the global config, which isn't very useful if you're using one runner in one project and another in another, e.g. the default runner for tests in one project and the eslint runner for linting in another).

Edit: added https://github.com/elyobo/jest-runner-oas-linter, just uses config in package.json or in a top level .oaslintrc.json.

@tunnckoCore
Copy link

tunnckoCore commented Jun 17, 2019

Still no solution? If anyway can guide me a bit from where we can start that would be great.

Otherwise, we should use hacky solutions. And yes, there is a way. And yes, this should land in runners implementations that you want to use, until the official jest support comes.

The solution is that runners should try to read from config.haste[runner-name]. Yes, it gives warnings, but the important thing is that it doesn't break the process.

Why in config.haste? Glad you asked. You can actually put whatever name you want in your jest config file (module.exports = { foobar: 123 }), but it won't appear in the jest runner (e.g. { config }, the config.foobar won't exist), but haste appears, always.

I've tested that solution and it's great for now. I probably will fork the babel and eslint runners for now if they don't accept my PRs.

@thymikee
Copy link
Collaborator

Please don't contribute hacks to the runners. Either apply the patch locally or maybe spend this time to actually implement the feature in Jest. You can start with normalize.ts file and see how other similar options are configured and pass the parameters down

@tunnckoCore
Copy link

tunnckoCore commented Jun 17, 2019

Yea, agree. Thanks for the starting point.

@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 26, 2022
@ljharb
Copy link
Contributor

ljharb commented Feb 26, 2022

bump

@github-actions github-actions bot removed the Stale label Feb 26, 2022
@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 26, 2023
@ljharb
Copy link
Contributor

ljharb commented Feb 26, 2023

bump

@github-actions github-actions bot removed the Stale label Feb 26, 2023
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 26, 2024
@ljharb
Copy link
Contributor

ljharb commented Feb 26, 2024

bump

@github-actions github-actions bot removed the Stale label Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants