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

[BUGFIX] Await potentially async operations #6301

Merged
merged 4 commits into from
Aug 15, 2019

Conversation

pzuraq
Copy link

@pzuraq pzuraq commented Aug 1, 2019

Some operations in Ember Data flush using run.join. This will flush
synchronously if no runloop or autorun currently exists, but will
join the existing runloop otherwise, making the scheduled task
effectively asynchronous.

With the tracked properties feature flag enabled, the way we've
scheduled autoruns has changed. A few Data tests were failing because
they were expecting an operation to flush synchronously, but it flushed
asynchronously due to an existing autorun. This PR adds appropriate
await settled(); statements in these cases.

@runspired
Copy link
Contributor

I opened emberjs/ember.js#18225 for the failing model tests, as it represents a regression in observer behavior that may have performance impacts.

@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2019

I'd rather update to https://github.com/emberjs/ember-qunit/releases/tag/v4.5.1 than add async to all the tests that aren't intentionally using asynchrony.

@runspired - This is the change we discussed in discord, basically in ember-qunit@4.5.1 we now allow a small "settling" period before triggering the test isolation errors.

@rwjblue
Copy link
Member

rwjblue commented Aug 14, 2019

#6325 does that ember-qunit update

Chris Garrett and others added 3 commits August 14, 2019 23:44
Some operations in Ember Data flush using `run.join`. This will flush
_synchronously_ if no runloop or autorun currently exists, but will
join the existing runloop otherwise, making the scheduled task
effectively asynchronous.

With the tracked properties feature flag enabled, the way we've
scheduled autoruns has changed. A few Data tests were failing because
they were expecting an operation to flush synchronously, but it flushed
asynchronously due to an existing autorun. This PR adds appropriate
`await settled();` statements in these cases.
@runspired runspired force-pushed the bugfix/await-potentially-async-operations branch from 99f95a4 to 1727a0e Compare August 15, 2019 06:44
@runspired
Copy link
Contributor

dropped commits for unnecessary async tests now that #6325 has landed

@runspired runspired force-pushed the bugfix/await-potentially-async-operations branch from f85c6c8 to 25fdc95 Compare August 15, 2019 07:32
@runspired runspired merged commit bde7c38 into master Aug 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the bugfix/await-potentially-async-operations branch August 15, 2019 07:53
@@ -367,7 +368,11 @@ module('unit/model/rollbackAttributes - model.rollbackAttributes()', function(ho
});

test("invalid record's attributes can be rollbacked", async function(assert) {
assert.expect(12);
if (gte('3.13.0')) {
Copy link
Member

Choose a reason for hiding this comment

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

I think these should be 3.13.0-beta.1, right? Otherwise it will continue to fail until 3.13.0 is released as stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gte treats beta as satisfying this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants