-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added failing test for #5111 #5157
Conversation
Is this bug still present in [email protected] ? |
4bbbec7
to
2eb64f1
Compare
@urbany Unfortunately so. I just rebased against master and pushed so you'll see a travis failure here shortly. I also ran the tests locally and this test is still failing. |
bfb9e58
to
7d5d942
Compare
run(() => { | ||
allPeople.objectAt(0).unloadRecord(); | ||
|
||
assert.equal(get(allPeople, 'length'), 2, 'Unload does not complete until the end of the loop'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we want this to happen before the loop ends to reduce # of wtfs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree that that would be more ideal. I had an extended conversation with @stefanpenner about that on slack. IIRC his near term objective was to get things back to how they were at 2.12, then later down the line rethink some of this stuff. That was several months back so it may have changed. But that was the thought process behind this test.
7d5d942
to
aa0d0bf
Compare
aa0d0bf
to
1db6b65
Compare
I've confirmed that #5378 will resolve this issue, and that PR has a test covering this case. Thanks so much for the failing test, made things easy to investigate! |
No description provided.