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

testSequencer config silently ignored in projects #9761

Closed
thernstig opened this issue Apr 3, 2020 · 15 comments · Fixed by #13565
Closed

testSequencer config silently ignored in projects #9761

thernstig opened this issue Apr 3, 2020 · 15 comments · Fixed by #13565

Comments

@thernstig
Copy link
Contributor

thernstig commented Apr 3, 2020

Note: This is fundamentally working as intended, this issue is now about validating the config and providing a sufficient warning.

🐛 Bug Report

The configuration testSequencer (https://jestjs.io/docs/en/configuration#testsequencer-string) is silently ignored in projects (https://jestjs.io/docs/en/configuration#projects-arraystring--projectconfig)

To Reproduce

Run this config and test sequencer:

module.exports = {
  projects: [
    {
      testEnvironment: 'node',
      testSequencer: './jestSequencer.js',
    },
  ],
};
const Sequencer = require('@jest/test-sequencer').default;

console.log('I will never get called.');

class CustomSequencer extends Sequencer {
  sort(tests) {
    // Test structure information
    // https://github.com/facebook/jest/blob/6b8b1404a1d9254e7d5d90a8934087a9c9899dab/packages/jest-runner/src/types.ts#L17-L21
    const copyTests = Array.from(tests);
    return copyTests.sort((testA, testB) => (testA.path > testB.path ? 1 : -1));
  }
}

module.exports = CustomSequencer;

Expected behavior

Jest should warn that the testSequencer should be global config, not per-project.
Original implementation: #6216

Link to repl or repo (highly encouraged)

See above.

envinfo

  System:
    OS: Linux 4.4 Ubuntu 16.04.5 LTS (Xenial Xerus)
    CPU: (4) x64 Intel(R) Xeon(R) Gold 6132 CPU @ 2.60GHz
  Binaries:
    Node: 12.13.0 - ~/.config/nvm/12.13.0/bin/node
    npm: 6.12.0 - ~/.config/nvm/12.13.0/bin/npm
  npmPackages:
    jest: 25.1.0 => 25.1.0
@jeysal
Copy link
Contributor

jeysal commented Apr 3, 2020

Thanks for the report. I would however say that it is intended that we sort allTests (as we do in the runJest function where the sequencer is called). We should reserve the right to interleave tests of different projects in any way necessary to optimize overall performance. In fact, being smarter about the order of running tests from different projects is something I've wanted to improve on for quite some time.
I would instead say the expected behavior should be that the config validation throws straight away, instead of silently ignoring the project-level test sequencer option. We can make this issue about fixing that (and label help wanted / good first issue). If someone then wants to sort per project, they can still use the global sequencer and implement it in a way that groups by test.context.config.name or something.

@jeysal
Copy link
Contributor

jeysal commented Apr 3, 2020

@thernstig would you be fine with that change? You could edit the issue or I can, whatever suits you :)

@thernstig
Copy link
Contributor Author

thernstig commented Apr 3, 2020

@jeysal What I am trying to achieve is to run end-to-end tests. Our end goal is to run them in parallel, but right now we want to run a login suite first (hence the sequencer) and then the rest (using a custom test runner with isSerial=true). Reading back on many issues from the past, this seem to be more and more common, seeing as Jest is awesome and thus users want to use it for also e2e tests.

Thus, using projects for this is nice if I could use a testSequencer just for that project, the e2e project. I am afraid that for me, that have not looked deeply into the what context is, and a quick google shows nothing, so I assume it would be undocumented and I would have to read into the source code and/or console.log() what's in there? (I did and just found a hash, so that did not bring me forward).

What is your suggestion to this problem then? I do understand the logic for properly sequencing is important for performance, but having a project used for e2e is a different beast.

@jeysal
Copy link
Contributor

jeysal commented Apr 3, 2020

Well, to be fair, manually writing a test sequencer is generally something that requires knowledge of some Jest internals, like writing a runner or an environment.
As for your use case, it's clear that you want to sort tests in the e2e in your custom way, and other tests in the regular @jest/test-sequencer way. Now ask yourself what should happen if a Jest run contains both e2e tests and other tests. If the answer is that other tests should run first and then e2e tests should run, I would recommend implementing your sequencer as follows:

  • Introspect test.context.config (the project config the test belongs to) to find out whether the test is e2e (this should be relatively easy, probably using rootDir)
  • If test1 and test2 are e2e, sort them in your custom way
  • If test1 and test2 are not e2e, delegate to @jest/test-sequencer (you can import and instantiate that to reuse its standard logic)
  • If test1 is e2e, but test2 is not, return 1.

@thernstig
Copy link
Contributor Author

thernstig commented Apr 3, 2020

Yes, fair points. I am pretty sure I can work around this regardless by introspecting everything in the sequencer. I've actually already got a way since I know the test case name for the login suite will be login.e2e.test.js I can just do:

    copyTests.sort((testA, testB) => {
      if (testA.path && testA.path.endsWith('login.e2e.test.js')) return -1;
      if (testB.path && testB.path.endsWith('login.e2e.test.js')) return 1;
      return 0;
    });

Problem solved! :)

I think this issue more is so that if you have a sequencer inside a project, you still want the main sequencer to sort in whatever way, and then the project sequencer sorts it an additional time for just the test cases in that project root. This would mean the main/top-level sequencer would sort "slowest first" (or any other performance enhancers) whilst the internal sort when will sort by name. The return 0 part will make sure all other sorts does not change the logic from the top-level sequencer which keeps the whole run performant.

I think I come from the perspective that e2e testing is common, jest awesome, and more users want to use Jest for it's power to run various kind of projects.

(no matter - throwing is better than anything even though I obviously wish there was a way to use a sequencer in projects as well 😄)

@jeysal
Copy link
Contributor

jeysal commented Apr 3, 2020

I think we want to keep the concept of a test sequencer very lightweight (like just swapping out the @jest/test-sequencer module for something else is). It was introduced very hesitantly after a lot of demand for being able to customize the order. One global sequencer has all the powers that individual project sequencers have (I gave an example), so no need to introduce more logic into Jest just for the customization use case.
Let's keep this issue open for the config validation part, I'll make a small edit.

@jeysal jeysal changed the title testSequencer config not working in projects testSequencer config silently ignored in projects Apr 3, 2020
@thernstig
Copy link
Contributor Author

thernstig commented Apr 3, 2020

Could you elaborate this part, for reference (for me and maybe others in the future) with some code? Appreciate it.

If test1 and test2 are not e2e, delegate to @jest/test-sequencer (you can import and instantiate that to reuse its standard logic)

@jeysal
Copy link
Contributor

jeysal commented May 25, 2020

Could you elaborate this part, for reference (for me and maybe others in the future) with some code?

Looks like it's not actually possible exactly the way I described it, since Jest's default sequencer does not expose its order function. But since sort is now pretty much guaranteed to be a stable sort in JS, you can first let Jest's default sequencer sort

import defaultSequencer from '@jest/test-sequencer';

// ... in your sort method
const preSortedTests = defaultSequencer.sort([...tests]);
// ... your custom sort on preSortedTests

and then do your sort - everything where you don't define an order (return 0) should remain in the order the standard Jest sequencer sorted it, your sequencer essentially just plucked out a few tests and moved them somewhere to the top or bottom.

@thernstig
Copy link
Contributor Author

thernstig commented May 26, 2020

Thanks @jeysal, already solved this some time ago by using the defaultSequencer's sort, worked great - even tested it to make sure it ran "slow/failed first". 😃

@anuragdw710
Copy link

hey, I want to contribute. please guide me on how to start contributing. I have fork and clone it and set the environment for this in my pc.

@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 Apr 17, 2022
@thernstig
Copy link
Contributor Author

hey, I want to contribute. please guide me on how to start contributing. I have fork and clone it and set the environment for this in my pc.

https://github.com/facebook/jest/blob/main/CONTRIBUTING.md

@ibuibu
Copy link
Contributor

ibuibu commented Nov 5, 2022

I challenge to contribute.

@SimenB
Copy link
Member

SimenB commented Nov 7, 2022

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

This issue 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 Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.