-
-
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
fix tests #7703
fix tests #7703
Changes from 4 commits
198eba7
d81f022
4f5fdca
2b22be8
af375be
8bd7d39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,10 +137,9 @@ module('integration/store - destroy', function (hooks) { | |
|
||
let store = this.owner.lookup('service:store'); | ||
let next; | ||
let nextPromise = new Promise((resolve) => (next = resolve)); | ||
let nextPromise; | ||
let TestAdapter = DS.Adapter.extend({ | ||
findRecord() { | ||
next(); | ||
nextPromise = new Promise((resolve) => { | ||
next = resolve; | ||
}).then(() => { | ||
|
@@ -165,7 +164,8 @@ module('integration/store - destroy', function (hooks) { | |
|
||
throw new Error("We shouldn't be pushing data into the store when it is destroyed"); | ||
}; | ||
let requestPromise = store.findRecord('car', '1'); | ||
|
||
store.findRecord('car', '1'); | ||
|
||
await nextPromise; | ||
|
||
|
@@ -175,14 +175,12 @@ module('integration/store - destroy', function (hooks) { | |
|
||
next(); | ||
|
||
await nextPromise; | ||
const result = await nextPromise; | ||
|
||
// ensure we allow the internal store promises | ||
// to flush, potentially pushing data into the store | ||
await settled(); | ||
assert.ok(true, 'we made it to the end'); | ||
await requestPromise; | ||
assert.ok(false, 'we should never make it here'); | ||
assert.equal(result.data.type, 'car', 'we made it to the end'); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is now failing on the main branch (see here where tests never finish). I rewrote it a little bit but the main issue was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this was failing on 3.20 (last 3.20 release was https://github.com/emberjs/ember.js/tree/v3.20.7) something has "changed" since the last PRs a week ago. In any case, I distilled the test down a bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha, interesting, it seems like we are asserting that the promise never finishes, it makes sense it would hang the test. We can rewrite this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with |
||
|
||
test('destroying the store correctly cleans everything up', async function (assert) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,10 +173,8 @@ export default { | |
SAMPLE_FEATURE_FLAG: null, | ||
RECORD_DATA_ERRORS: true, | ||
RECORD_DATA_STATE: true, | ||
IDENTIFIERS: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we might want to keep these, in case someone was reading them and branching code off of it, removing them would break. Do you think there is a downside in not GCing them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only thought to commit these for clarity of the codebase. Certainly if someone was relying on a forked version of e-d, but is it something we expect users to read? And if they do, then it is expected they could be removed at any point? As long as we get the tests passing, I'm ok to do whatever you think is best 👍 |
||
REQUEST_SERVICE: true, | ||
CUSTOM_MODEL_CLASS: true, | ||
FULL_LINKS_ON_RELATIONSHIPS: true, | ||
RECORD_ARRAY_MANAGER_IDENTIFIERS: true, | ||
REMOVE_RECORD_ARRAY_MANAGER_LEGACY_COMPAT: true, | ||
}; |
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 thought I was reading this right but now I'm questioning myself. I wouldn't think we would advance from this line because we have yet to
resolve
the promise. I thought it would just hang here.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.
It gets resolved on line
143
no?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.
Here is a plan TS example. We haven't resolved yet as of our
await nextPromise
. Clearly a gap in my knowledge somewhere 😆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 missed that store.findRecord would batch multiple if necessary. As a result, nextPromise is undefined here.