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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override global jasmine spec functions #18917

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
5 participants
@rafeca
Copy link
Contributor

commented Feb 26, 2019

Issue or RFC Endorsed by Atom's Maintainers

Comment on previous PR.

Description of the Change

Currently, if a spec uses the standard it function on an async test (the one that's stored in the global object), that test will always pass. This happens because the jasmine version checked into Atom does not natively support tests that return promises, so the failure happens asynchronously and the test runner cannot detect it.

To overcome this limitation, and because of the many repos that Atom use, a bunch of async-spec-helpers.js files have emerged (right now there are 31 of them) which implement custom it, fit, beforeEach, afterEach,... methods with support for async tests.

This can be confusing since some tests are currently using the builtin jasmine methods (mostly old tests that don't use async functions yet) and some other tests use the overriden versions located across the different async-spec-helpers.js files.

This PR modifies the builtin jasmine functions on the global test runner, so any Atom test will be able to use async functions, making the behaviour uniform across all tests and addressing the valid concern stated by @Arcanemagus in #18896 .

As a nice side-effect, this PR will allow us to remove many of the async-spec-helpers.js files that we have hanging around 馃槂

Alternate Designs

Ideally we should upgrade the version of Jasmine that Atom tests are using: since v2.7 Jasmine supports async tests.

Doing the upgrade may be very tricky, since Atom uses its own checked in version of Jasmine which has local modifications and hasn't been updated since 6 years ago.

In the case that we want to do an upgrade of the test runner, it would be worth to explore how feasible would be to switch to a more advanced test runner like Jest which will probably provide many benefits like reduced test times or better developer experience (though it would be tricky to integrate since Atom tests are run in the Atom runtime, while Jest currently only runs on Node.js).

Possible Drawbacks

Overriding globals is ugly and I should feel bad about it 馃ぁ

Verification Process

Check that all tests in CI keep passing.

Release Notes

N/A.

@alawi247

This comment was marked as spam.

Copy link

commented Feb 26, 2019

need help

@rafeca rafeca force-pushed the rafeca:rewrite-jasmine-globals branch 3 times, most recently from dd6de73 to e70a496 Feb 27, 2019

@rafeca rafeca marked this pull request as ready for review Feb 27, 2019

@rafeca rafeca force-pushed the rafeca:rewrite-jasmine-globals branch from e70a496 to ee1c7c8 Feb 27, 2019

@jasonrudolph
Copy link
Member

left a comment

Thanks for taking this on!

I have one question about the implementation, but this is looking good overall. If it would be easier to to talk through this over video chat, just let me know, and we'll make it happen. 馃槃

Show resolved Hide resolved spec/jasmine-test-runner.coffee

@rafeca rafeca force-pushed the rafeca:rewrite-jasmine-globals branch from ee1c7c8 to ea24038 Feb 27, 2019

@jasonrudolph jasonrudolph requested a review from Arcanemagus Feb 27, 2019

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@Arcanemagus: Given your familiarity with this space, would you mind reviewing this pull request. If these changes look good to you, I'm 馃憤 on merging.

@Arcanemagus
Copy link
Contributor

left a comment

Tested Atom using the code from this branch and confirmed that this does indeed translate over to allowing any package spec to use async/await.

I had to add this block (source) to a few of the test package's beforeEach blocks, but I think this is to be expected and not something we want to mess with here because the timing stuff is actually utilized for the specs here:

Object.keys(jasmine.Clock.real).forEach((key) => {
  window[key] = jasmine.Clock.real[key];
})
jasmine.unspy(Date, 'now');

rafeca added a commit to atom/snippets that referenced this pull request Feb 28, 2019

Don't return an unresolved Promise on beforeEach()
The `editor.update()` method returns a Promise, which under some
circumstances (on specs mostly) never gets resolved.

Because of CoffeeScript's implicit returns, this Promise was returned on
the `beforeEach()` method of these tests.

Before atom/atom#18917, any Promise that was
returned by a spec method was just discarded, so this behaviour didn't
cause any issue, but once we merge that PR this test is going to start
failing because jasmine will wait indefinitely until the Promise is
resolved.
@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Note that this PR can potentially cause failures on tests that were passing previously (an example is the one I fixed in atom/snippets#290 ).

This can happen whenever an error was swallowed by a Promise that was not awaited or whenever a Promise that never gets resolved is returned on a spec method. IMHO failing in these situations is a good thing, and so far the only test that I've seen failing because of this is the one that I fixed on atom/snippets#290, so I'm gonna go ahead and merge this PR.

@rafeca rafeca self-assigned this Feb 28, 2019

@rafeca rafeca referenced this pull request Feb 28, 2019

Merged

猬嗭笍 snippets@1.4.1 #18923

@rafeca rafeca force-pushed the rafeca:rewrite-jasmine-globals branch from ea24038 to b8953f2 Feb 28, 2019

@rafeca rafeca force-pushed the rafeca:rewrite-jasmine-globals branch from b8953f2 to c5bd128 Feb 28, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: #18896 (comment)

@rafeca rafeca force-pushed the rafeca:rewrite-jasmine-globals branch from c5bd128 to 7876e04 Feb 28, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

I'm gonna land this PR with the failing test (it's a flaky check and after 3 retries I keep getting different failures)

@rafeca rafeca merged commit c4aa086 into atom:master Feb 28, 2019

2 of 3 checks passed

Atom Pull Requests #20190228.11 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the rafeca:rewrite-jasmine-globals branch Feb 28, 2019

@cpojer

This comment has been minimized.

Copy link

commented Mar 5, 2019

@rafeca If you are considering using Jest for Atom, I recommend checking out the electron runner: https://github.com/facebook-atom/jest-electron-runner

We use this at FB for running Nuclide's tests using Jest. It works!

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Hii @cpojer! 馃憢

Wow that looks promising! I'll take a look and check with @aaronabramov 馃槂

Thanks for the pointer!

rafeca added a commit to atom/fuzzy-finder that referenced this pull request Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.