-
-
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
Conversation
Asset Size Report for 8bd7d39 IE11 Builds ☑️ EmberData has not changed in sizeIf any packages had changed sizes they would be listed here. Changeset
Full Asset Analysis (IE11)
Modern Builds ☑️ EmberData has not changed in sizeIf any packages had changed sizes they would be listed here. Changeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) ☑️ EmberData has not changed in sizeIf any packages had changed sizes they would be listed here. Changeset
Full Asset Analysis (Modern)
|
6a2c991
to
509d9ce
Compare
Performance Report for 8bd7d39 Scenario - materialization: ☑️ Performance is stable
Scenario - unload: ☑️ Performance is stable
Scenario - destroy: ☑️ Performance is stable
Scenario - add-children: ☑️ Performance is stable
Scenario - unused-relationships: ☑️ Performance is stable
|
2adc748
to
d81f022
Compare
54ac421
to
4f5fdca
Compare
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 comment
The 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 await requestPromise
. On first glance, I can't tell what is going wrong 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.
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 comment
The 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 .then
and not hang on it, or just rely on the store.push
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.
Fixed with .then
! Thank you!
@@ -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; |
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.
@snewcomer Any plans on upping the tiers on your sponsor page? runspired has higher-tiers available (https://github.com/sponsors/runspired), and would be happy to up our sponsorship for you as well. May I suggest a $100 tier? |
@@ -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 comment
The 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 comment
The 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 👍
* chore(deps): bump broccoli-rollup from 4.1.1 to 5.0.0 (#7559) Bumps [broccoli-rollup](https://github.com/chadhietala/broccoli-rollup) from 4.1.1 to 5.0.0. - [Release notes](https://github.com/chadhietala/broccoli-rollup/releases) - [Changelog](https://github.com/chadhietala/broccoli-rollup/blob/master/CHANGELOG.md) - [Commits](https://github.com/chadhietala/broccoli-rollup/commits/v5.0.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * CI timeouts testing bump (#7734) * Fix hanging store.destroy testt (#7703) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
further cleanup of #7393