-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
test(repository-tests): adding acceptance tests for hasManyThrough relation #5765
Conversation
7eb708d
to
02380ed
Compare
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.
👏 Glad to see HMT pass acceptance tests for connectors. LGTM!
I left a few comments for advanced test cases, if they are valid, feel free to address them in separate PRs.
packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts
Show resolved
Hide resolved
|
||
await expect( | ||
customerRepo.cartItems(existingCustomerId).patch({id: anotherId}), | ||
).to.be.rejectedWith(/Property "id" cannot be changed!/); |
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.
👍
expect(links).have.length(2); | ||
expect(cartItems).have.length(2); | ||
|
||
await customerRepo.cartItems(existingCustomerId).delete(); |
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.
nitpick: does delete
takes in a filter? Maybe add another test for delete with filter
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.
Just realized that the delete
method has a bug. It is able to delete targets based on the filter but it deletes all through instance that has the souceKey. Will open up an issue to fix it.
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.
The issue is being tracked in #5852
#4438 #2359
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈