[CHORE]: Modernized store-adapter-test #7634
Conversation
snewcomer
added
🏷️ chore
| this.owner.register('model:person', Person); | ||
| this.owner.register('model:dog', Dog); | ||
| this.owner.register('adapter:application', JSONAPIAdapter.extend()); | ||
| this.owner.register('serializer:application', JSONAPISerializer.extend()); |
There was a problem hiding this comment.
Did you find that a beforeEach was too overbearing and it was easier to do it in the specific tests that need it?
There was a problem hiding this comment.
Yes, keeping in the spirit of defining individually per test. What do you think of that design choice, has removing beforeEach made tests on the whole too verbose?
There was a problem hiding this comment.
When writing tests, it probably is preferred to take common test setup and only repeat yourself once (or a few times if you need to make some modifications for a few tests). In this example, we might be able to reduce a few hundred lines of code!
There was a problem hiding this comment.
Thanks for the feedback, I am in complete agreement about minimizing redundant code and making the file much smaller! I was basing this change on: #7567 (review)
Why was it more preferred to eliminate the beforeEach in that PR?
There was a problem hiding this comment.
Great question. I think it was a bit more nuanced in that example. In that PR, it was re-declaring Post (via Post = ...). Effectively taking over Post from something that had greater scope access. In this case, the thing in the beforeEach here is necessary setup for each test. From looking at the tests briefly, we just may want the most broad test setup. So that looks like a Person with 2 attributes and a relationship....As well as a Dog with one attribute.
There was a problem hiding this comment.
I see, great explanation thank you! I'll roll this back a couple of commits and put the beforeEach back in
snewcomer
force-pushed
the
store-adapter-test
branch
from
1f4edcf
to
185044c
Compare
runspired
force-pushed
the
store-adapter-test
branch
from
185044c
to
cdc8a1f
Compare
runspired
force-pushed
the
store-adapter-test
branch
from
cdc8a1f
to
a66c300
Compare
No description provided.