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

Implement test suite errors #44

Closed
wants to merge 3 commits into from
Closed

Implement test suite errors #44

wants to merge 3 commits into from

Conversation

samueltklaw
Copy link

@samueltklaw samueltklaw commented Mar 23, 2018

This change was made to capture and output test errors (as opposed to failures) when a test is invalid for various reasons (for example, broken test files).
Previously if a test suite has errored out, jest-junit would display that no tests ran and no test failed, which may incorrectly imply that nothing went wrong.

Please let me know if you have further questions.
Thanks!

@palmerj3
Copy link
Collaborator

@stklaw thank you!

I've thought about this for a while and I'm glad you are tackling it!

My reason for not merging it now, however, is that it does not detect how many errors there are. The value is only 0 or 1.

I believe to get the real number jest would have to provide more data to testResultsProcessors.

What do you think?

@samueltklaw
Copy link
Author

@palmerj3 Indeed. I've been investigating a bit before creating this PR and I haven't found a reliable way to detect number of errors, since the executor just short-circuits upon encountering exceptions. For my particular use case, all tests within a test suite was tied to a particular test file, so it didn't really matter how many tests that erroring suite contained. If there is a way to do it, perhaps it might be worth expanding upon, but I haven't had that need in my case.

This is just my opinion but I think the value of this PR is being able to know that something failed at all so that the tester may investigate more deeply.

@samueltklaw
Copy link
Author

@palmerj3 Just to clarify, are you referring to the test suite or the test cases regarding the error value?

@palmerj3
Copy link
Collaborator

@stklaw yeah I think for a testResultsProcessor we'd need to be able to show whether an individual test case failed in error and the total number of test case errors in order to show in the test suite

@samueltklaw
Copy link
Author

samueltklaw commented Mar 27, 2018

@palmerj3 Hi again. I did a bit of research about this issue over the weekend and based on what I found, I think that my implementation should be acceptable. I'll try to summarize and justify myself a bit here. I apologize if it's a bit lengthy.

In general, I found 3 kinds of scenarios in regards to errors:

  1. Unhandled runtime errors within test cases
  2. Malformed tests cases (syntax errors)
  3. No test cases

Consider the following example:

describe('example test suite', () => {    
    it('should fail (TypeError)', () => {
        null.f();
        expect(true).toBe(true);
    });
    it('should fail (throw)', () => {
        throw ReferenceError();
        expect(true).toBe(true);
    });
    it('should pass', () => {
        expect(true).toBe(true);
    });
});

In this scenario, Jest will return a report with 2 failing tests and 1 passing test, since Jest deems runtime exceptions thrown inside test cases to be failures. From what I found from testing and digging into Jest's source, they do not distinguish errors and failures for individual test cases (https://github.com/facebook/jest/blob/master/packages/jest-cli/src/test_result_helpers.js). Because of this, there's no way for us to assign error stats to test cases unless we change the report from Jest.

Malformed tests were what I had in mind when I created this PR. For malformed tests, the scenario may look something like this:

describe('example test suite', () => {    
    it('should pass', () => {
        expect(true).toBe(true);
    });
    it('should break everything', () => {
        &&&
    });
    it('should break everything too', () => {
        SomeTestFunction();
    });
});
const SomeTestFunction = () => {
    &&&
}

In this scenario, there is a syntax error and the Jest does not attempt to run any tests at all, even though there are other working tests. Jest simply outputs an empty test suite with a property called testExecError (I updated my implementation to check to for this property instead). The report here we get 1 failing test suite with 0 passing tests and 0 failing tests. Since jest-junit only outputs the count for test cases, it can be deceiving, especially when running many test suites, it can be easy to miss. This is the scenario that I am most concerned about because it hides the existence of the other tests inside the test suite. With this change, the junit.xml will display tests="0" failures="0" errors="1" instead.

The last scenario with empty test suites is somewhat trivial, so I won't go into detail about it unless you'd like to talk more about it.

Sorry if this was a mess, I'm still new to Jest and this is my first public contributions on github! :)
Let me know what you think or if there's need for improvement!
Thanks

@palmerj3
Copy link
Collaborator

Closing this PR for now. Please follow this issue on jest since I suspect we'll have a more permanent fix soon. jestjs/jest#5958

@palmerj3 palmerj3 closed this Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants