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

Fix crash when Promise rejects with null #6897

Closed
wants to merge 3 commits into from

Conversation

Pajn
Copy link
Contributor

@Pajn Pajn commented Aug 27, 2018

Fixes #6896
Jest crashed / Shows test failure from exception inside Kest

it('should just fail the test', () => {
  return Promise.reject(null)
})

Test story:

Help wanted!
How do I test this? Adding a simple unittest to is_error does not really show how it fixes the actual problem.

@codecov-io
Copy link

Codecov Report

Merging #6897 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6897   +/-   ##
=======================================
  Coverage   66.98%   66.98%           
=======================================
  Files         250      250           
  Lines       10358    10358           
  Branches        3        3           
=======================================
  Hits         6938     6938           
  Misses       3419     3419           
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/is_error.js 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c93e027...d95f459. Read the comment docs.

@natealcedo
Copy link

natealcedo commented Sep 1, 2018

Hey @Pajn, I'm not a core contributor on this project so take my advice with a grain of salt, but here's my 2 cents. I suggest you go down the path of writing an end to end test for this. I've tried this locally with your changes and this test works.

This file is located jest/e2e/__tests__/promise_reject.test.js and I have a folder named jest/e2e/promise-reject

/**
 * Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

'use strict';

import runJest from '../runJest';
import path from 'path';
const {cleanup, writeFiles} = require('../Utils');

const DIR = path.resolve('../promise-reject');

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

test('', () => {
  writeFiles(DIR, {
    'package.json': '{}',
    'promise_reject.test.js': `
      test('test', () => {
        return Promise.reject(null)
      });
    `,
  });

  const {stdout, stderr, status} = runJest(DIR);

  expect(stdout).toBe('');
  expect(stderr).toMatch(/Failed: null/);
  expect(status).toBe(1);
});

Perhaps one of the senior contributors can weigh in on this?

@SimenB
Copy link
Member

SimenB commented Sep 1, 2018

You can do JEST_CIRCUS=1 yarn jest ... to run the test with circus (the CI failure). It should be a matter of normalizing the output before the assertion.


Thanks for weighing in @natealcedo!

@natealcedo
Copy link

Hey @Pajn, do you still wanna work on this? I don't mind carrying on from here :)

@Pajn
Copy link
Contributor Author

Pajn commented Sep 25, 2018

Sorry, I forgot to update here and then forgot the whole issue.

I had trouble finding where to make the change. Please take over if you want as you probably can navigate the code much better than I.

@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 May 12, 2021
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.

Crash when potentialError is null
5 participants