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

Don't return an unresolved Promise on beforeEach() #290

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
1 participant
@rafeca
Copy link
Contributor

commented Feb 28, 2019

Description of the Change

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.

Alternate Designs

Figure out why the Promise that gets returned by editor.update() never gets resolved. I've started investigating it and this only happens when this codepath gets executed.

The official tests for TextEditor seem to avoid awaiting for that Promise (e.g if we add an await in this line that test is going to fail), so I've left the proper investigation in a TODO comment.

Benefits

This unblocks merging atom/atom#18917

Possible Drawbacks

N/A

Applicable Issues

N/A

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

I'm gonna merge this without a review since it's a safe change and it unblocks atom/atom#18917 🙃

@rafeca rafeca merged commit 18cc1de into master Feb 28, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the fix-unresolved-promise branch Feb 28, 2019

@rafeca rafeca referenced this pull request Feb 28, 2019

Merged

⬆️ snippets@1.4.1 #18923

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