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

polymorphic tests don't actually implement polymorphism #449

Open
runspired opened this issue Apr 29, 2021 · 3 comments
Open

polymorphic tests don't actually implement polymorphism #449

runspired opened this issue Apr 29, 2021 · 3 comments
Assignees
Labels

Comments

@runspired
Copy link
Contributor

The model homepage-item is expecting records that implement the itemizeable type, however no records do.

The included polymorphic test only checks whether these records can be loaded, it doesn't check the hasMany relationships state.

While working on relationships in ember-data I recently noticed that our polymorphic assertion does not currently run for canonical/remote state updates (but does run for all forms of local state updates). Adding this missing assertion causes this test to fail.

I'm opening this issue because while I think the test should correctly implement the current polymorphic setup requirements (e.g. post and comment should implement itemizeable via shared base class or mixin) the current work I am doing is intended to firm up some things that would allow us to utilize polymorphic relationships without mixins or inheritance. E.g. in a future state, because this relationship has no inverse defined on post or comment (or even on the itemizeable mixin) then this setup could possibly be considered correct.

See emberjs/data#7491 for the PR which uncovered this.

@ryanto ryanto self-assigned this Apr 30, 2021
@ryanto ryanto added the Bug label Apr 30, 2021
@ryanto
Copy link
Member

ryanto commented May 1, 2021

Thanks, agree and surprised itemizable isn't being used anywhere.

What's the best way for me to see this assertion fail with storefront's test suite? When I run ember try:one ember-canary --- ember test all tests pass.

@runspired
Copy link
Contributor Author

@ryanto we have auto-publishing canary versions now but I think it runs once mid-week, I'll publish one for you now because else you have to run the script from ember-data's side that creates the tarballs and inserts them into this package.

@runspired
Copy link
Contributor Author

You can also see a failed run here: https://github.com/emberjs/data/runs/2479805903

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

No branches or pull requests

2 participants