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

Handling pending calls inside async tests properly. #6782

Merged
merged 4 commits into from
Oct 8, 2018

Conversation

matrushka
Copy link
Contributor

Summary

Currently pending calls inside async tests causes the test to fail instead of getting skipped. Example:

it('should be pending', async () => {
  pending('skip');
});

With this PR pending calls inside async tests are handled properly as skipped tests.

Test plan

Was not able to find any tests for the current logic so was not able to figure out the test plan and did a manual testing. If someone can guide me about how to test this I'm willing to write the tests for it.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

codecov-io commented Jul 30, 2018

Codecov Report

Merging #6782 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6782      +/-   ##
=========================================
- Coverage   66.61%   66.6%   -0.02%     
=========================================
  Files         253     253              
  Lines       10626   10629       +3     
  Branches        4       4              
=========================================
  Hits         7079    7079              
- Misses       3546    3549       +3     
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/jasmine_async.js 0% <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 2d972e5...f440020. Read the comment docs.

@thymikee
Copy link
Collaborator

thymikee commented Aug 7, 2018

We're moving towards dropping Jasmine, so not sure if we want this change. @aaronabramov wdyt?

@SimenB
Copy link
Member

SimenB commented Aug 15, 2018

If we don't wanna do this we should document the fact that jasmine is in maintenance and we'll just fix breaking bugs

@SimenB
Copy link
Member

SimenB commented Oct 7, 2018

Doesn't hurt to fix this, I guess.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing an integration test. See contributing.md for information on how to write one.

Also, please update the changelog

@matrushka
Copy link
Contributor Author

@SimenB I've done the changes but I guess circus doesn't have a pending method which causes the new test to fail in circus. How should I proceed?

@SimenB
Copy link
Member

SimenB commented Oct 8, 2018

@matrushka
Copy link
Contributor Author

matrushka commented Oct 8, 2018

Which one will you prefer?

  1. Skipping the whole jasmine_async e2e suite with skipSuiteOnJestCircus()
  2. Wrapping the new test with aisJestCircusRun() conditional
  3. Moving the new test out to a separate file which has skipSuiteOnJestCircus()

@SimenB
Copy link
Member

SimenB commented Oct 8, 2018

describe('global pending', () => {
  skipSuiteOnJestCircus();

  it('tests async promise when it skips during the test', () => {
    const result = runWithJson('jasmine-async', ['pending_in_promise.test.js']);
    const json = result.json;
    expect(json.numPendingTests).toBe(1);
  });
});

Or something like that? Whatever's cleanest

* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @jest-environment jsdom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed

@matrushka
Copy link
Contributor Author

@SimenB done with the test as well. Moved it out to avoid changing previous tests and now it passes all checks.

@SimenB SimenB merged commit c4646b9 into jestjs:master Oct 8, 2018
@SimenB
Copy link
Member

SimenB commented Oct 8, 2018

Thanks!

@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.

5 participants