Proposed changes for malformed tests

[rossknudsen]

🚀 Feature Proposal

A change to the way that malformed tests are handled (e.g. don't have expected arguments) by running as much of the code as possible to provide the user as much useful information about what went wrong and how to fix it.

Motivation

This is dealing with an edge case, where tests have not been implemented correctly. This typically is due to missing arguments, or using the wrong argument types. Currently the tests stop executing at the first error that is encountered in a test file. I think it would be more helpful to the user if they had feedback on all potential issues in a single test run, rather than only being directed to the first issue.

Example

Here are some examples of malformed tests:

describe("tests with missing callbacks", () => {
    test("test with missing callback")
    // make sure that multiple tests are processed.
    test("test with missing callback 2")
})

describe("describe with missing callback")

// make sure that multiple describes are processed.
describe("describe with missing callback 2")

// this doesn't emit anything.  Probably acceptable for now.
describe("describe no tests", () => {
})

// confirm that can still process valid tests.
describe("describe with callback and tests", () => {
    it("test should pass", () => {})
    it("test should pass 2", () => {})
})

This is the current output:

yarn run v1.19.1
$ jest
 FAIL  src/__tests__/newTest.test.ts
  ● Test suite failed to run

    Missing second argument. It must be a callback function.

       5 | })
       6 | 
    >  7 | describe("describe with missing callback")
         | ^
       8 | 
       9 | // make sure that multiple describes are processed.
      10 | describe("describe with missing callback 2")

      at Env.describe (../../Documents/source/jest/packages/jest-jasmine2/build/jasmine/Env.js:390:19)
      at Object.describe (src/__tests__/newTest.test.ts:7:1)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        6.592s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Pitch

Notice how the execution of the tests is halted at the first error in the file. Personally I would like it if it ran as much of the code as possible, highlighting all the areas that there are issues. Obviously this excludes failures in syntax (missing braces/parens etc).

Here is the output with my proposed changes applied (same test code):

yarn run v1.19.1
$ jest
 FAIL  src/__tests__/newTest.test.ts
  tests with missing callbacks
    × test with missing callback (4ms)
    × test with missing callback 2 (11ms)
  describe with missing callback
    × encountered a declaration exception (1ms)
  describe with missing callback 2
    × encountered a declaration exception (1ms)
  describe with callback and tests
    √ test should pass
    √ test should pass 2

  ● tests with missing callbacks › test with missing callback

    Missing second argument. It must be a callback function. Perhaps you want to use `test.todo` for a test placeholder.

      1 | describe("tests with missing callbacks", () => {
    > 2 |     test("test with missing callback")
        |     ^
      3 |     // make sure that multiple tests are processed.
      4 |     test("test with missing callback 2")
      5 | })

      at Env.it (../../Documents/source/jest/packages/jest-jasmine2/build/jasmine/Env.js:586:21)
      at Suite.test (src/__tests__/newTest.test.ts:2:5)
      at Object.describe (src/__tests__/newTest.test.ts:1:1)

  ● tests with missing callbacks › test with missing callback 2

    Missing second argument. It must be a callback function. Perhaps you want to use `test.todo` for a test placeholder.

      2 |     test("test with missing callback")
      3 |     // make sure that multiple tests are processed.
    > 4 |     test("test with missing callback 2")
        |     ^
      5 | })
      6 | 
      7 | describe("describe with missing callback")

      at Env.it (../../Documents/source/jest/packages/jest-jasmine2/build/jasmine/Env.js:586:21)
      at Suite.test (src/__tests__/newTest.test.ts:4:5)
      at Object.describe (src/__tests__/newTest.test.ts:1:1)

  ● describe with missing callback › encountered a declaration exception

    Missing second argument. It must be a callback function.

       5 | })
       6 | 
    >  7 | describe("describe with missing callback")
         | ^
       8 | 
       9 | // make sure that multiple describes are processed.
      10 | describe("describe with missing callback 2")

      at Env.describe (../../Documents/source/jest/packages/jest-jasmine2/build/jasmine/Env.js:391:21)
      at Object.describe (src/__tests__/newTest.test.ts:7:1)

  ● describe with missing callback 2 › encountered a declaration exception

    Missing second argument. It must be a callback function.

       8 | 
       9 | // make sure that multiple describes are processed.
    > 10 | describe("describe with missing callback 2")
         | ^
      11 | 
      12 | // this doesn't emit anything.  Probably acceptable for now.
      13 | describe("describe no tests", () => {

      at Env.describe (../../Documents/source/jest/packages/jest-jasmine2/build/jasmine/Env.js:391:21)
      at Object.describe (src/__tests__/newTest.test.ts:10:1)

Test Suites: 1 failed, 1 total
Tests:       4 failed, 2 passed, 6 total
Snapshots:   0 total
Time:        5.242s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[SimenB]

seems reasonable. Right now we throw, which is why it stops where it does. Instead marking the tests as failing and moving on might make sense. Would you be willing to work on this feature? At least for circus, I don't really care about Jasmine.

Current throwing logic lives here: https://github.com/facebook/jest/blob/964dc1619b8c5345a648ef79bcd87c087995255a/packages/jest-circus/src/index.ts
Dispatching a new event instead of throwing, then adding test errors afterwards might work?

[rossknudsen]

Would you be willing to work on this feature?

Yes, I'm keen to have a go at it. I have already played with the code, but in jest-Jasmine2 rather than where you've linked. So I'll have to figure out what the difference is.

[SimenB]

If you're willing to also fix it in jest-jasmine that's great 👍 Any changes must also land in circus though, as we'll be deprecating the jasmine runner at some point 🙂

[jeysal]

Any questions about what jasmine/circus are / how they relate to each other let us know, this has been confusing for many contributors

[SimenB]

Might be an idea to write some prose for the website

(^ not me volunteering)

[rossknudsen]

Hey quick question. I'm working on updating the unit tests for this change in behaviour and I'm hitting an issue. Jest prevents you from nesting a test within another test. There are existing unit tests for the it/test function e.g.:

it(`it throws error with missing callback function`, () => {
    expect(() => {
      it('test1');
    }).toThrowError(
      'Missing second argument. It must be a callback function. Perhaps you want to use `test.todo` for a test placeholder.',
    );
  });

This works fine because the existing implementation throws the exception as expected. However, because I'm now changing that behaviour to not throw, I'm now hitting an issue with testing a test framework with itself. Jest prevents tests from being nested and the nesting check is performed after the code previously would have thrown the expected error, which is why the test works.

Note that this is for the Jasmine side of things. Circus it/test is tested with Jasmine so it gets around the nesting issue.

Any thoughts on how to fix this?

[rossknudsen]

I think I'll ensure that the e2e tests cover this and remove the unit tests.

[jeysal]

Yes, I think removing this unit test is fine (as 'throws with missing callback function' is no longer the desired behavior, it should now 'collect the missing callback function error and proceed and throw it at the end') as long as a new test (e2e/unit/whatever kind) is added :)
Sorry for the loooooong response time btw 🙈

[github-actions]

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 14 days.

[github-actions]

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.