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.

Sign up for GitHub

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

Jump to bottom

[CHORE] Refactor away from createStore test helper #6347

Merged
merged 1 commit into from
Aug 22, 2019
Merged

[CHORE] Refactor away from createStore test helper #6347

merged 1 commit into from
Aug 22, 2019

Conversation

HeroicEric
Copy link
Member

From #6166

This refactors some tests so that they don't rely on the createStore test helper.

createStore was helpful in the past but is less so now that we have nice APIs for dependency injection.

One test was removed from unit/store/adapter-interop - Store working with a Adapter because I don't think it was correct.

Here was the test:

data/packages/-ember-data/tests/unit/store/adapter-interop-test.js

Lines 28 to 32 in 39182f3

test('Adapter can be set as a factory', function(assert) {
store = createStore({ adapter: TestAdapter });
assert.ok(store.get('defaultAdapter') instanceof TestAdapter);
});

This test was, at one time, testing that it was possible to pass a factory as the adapter property to Store. This feature was removed in #3191 but the test continued passing because of the way the createStore works.

data/packages/-ember-data/tests/helpers/store.js

Lines 57 to 60 in 39182f3

if (typeof adapter !== 'string') {
env.registry.register('adapter:-ember-data-test-custom', adapter);
adapter = '-ember-data-test-custom';
}

@HeroicEric
Copy link
Member Author

If this looks good I can do the same for setupStore

snewcomer
snewcomer reviewed Aug 20, 2019
View reviewed changes
Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

packages/-ember-data/tests/unit/model/merge-test.js Show resolved Hide resolved
packages/-ember-data/tests/unit/model/lifecycle-callbacks-test.js Show resolved Hide resolved
packages/-ember-data/tests/unit/adapters/rest-adapter/group-records-for-find-many-test.js
testRecord: DS.Model.extend(),
});
});
this.owner.register('adapter:application', ApplicationAdapter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any cases where we have to register the corresponding RESTSerializer? (previously the createStore was doing this)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The createStore helper was actually registering the -default serializer as the JSONAPISerializer:

data/packages/-ember-data/tests/helpers/store.js

Line 70 in 7c7ebdf

owner.register('serializer:-default', JSONAPISerializer);

There were a few cases where I needed to do this where the test involves mocking the response from the adapter.
Some examples:

https://github.com/emberjs/data/pull/6347/files#diff-8aaaee520d117ac876f402e7a684ebd8R34
https://github.com/emberjs/data/pull/6347/files#diff-8aaaee520d117ac876f402e7a684ebd8R101
https://github.com/emberjs/data/pull/6347/files#diff-8aaaee520d117ac876f402e7a684ebd8R189

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my notes in setupStore in #6353 for providing serializers. TL;DR we should always be supplying a serializer in tests unless we are testing the current default logic.

@runspired runspired added the 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166 label Aug 20, 2019
@runspired runspired mentioned this pull request Aug 20, 2019
Merged
runspired
runspired approved these changes Aug 21, 2019
View reviewed changes
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the focus on eliminating createStore, I'm alright with this being a slightly more minimal refactor away from createStore vs fully modernizing all of the tests it touches. We do want to eliminate the usage of run and reopen but can do that in a follow up.

Left some comments for minor nits.

packages/-ember-data/tests/unit/adapters/rest-adapter/group-records-for-find-many-test.js
testRecord: DS.Model.extend(),
});
});
this.owner.register('adapter:application', ApplicationAdapter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my notes in setupStore in #6353 for providing serializers. TL;DR we should always be supplying a serializer in tests unless we are testing the current default logic.

packages/-ember-data/tests/unit/debug-test.js Outdated Show resolved Hide resolved
packages/-ember-data/tests/unit/store/has-model-for-test.js Show resolved Hide resolved
packages/-ember-data/tests/unit/store/unload-test.js
Record = DS.Model.extend({
title: DS.attr('string'),
wasFetched: DS.attr('boolean'),
let Record = Model.extend({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer const in this situation as it would be a mistake to reassign

@HeroicEric
[CHORE] Refactor away from createStore test helper
0f2acb6 
From emberjs#6166

This refactors some tests so that they don't rely on the `createStore`
test helper.

`createStore` was helpful in the past but is less so now that we have
[nice APIs for dependency injection](https://guides.emberjs.com/release/applications/dependency-injection/).

---

One test was removed from `unit/store/adapter-interop - Store working
with a Adapter` because I don't think it was correct.

Here was the test:

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/unit/store/adapter-interop-test.js#L28-L32

This test was, at one time, testing that it was possible to pass a
factory as the `adapter` property to `Store`. This feature was removed
in emberjs#3191 but the test continued
passing because of the way the `createStore` works.

https://github.com/emberjs/data/blob/39182f324d5777e4a75c4a582f0791631e37d8de/packages/-ember-data/tests/helpers/store.js#L57-L60
@runspired runspired merged commit 4aa8e41 into emberjs:master Aug 22, 2019
HeroicEric added a commit to HeroicEric/data that referenced this pull request Aug 22, 2019
@HeroicEric
[CHORE tests] Replace setupStore for relationship integration tests
0acfd40 
Part of emberjs#6166
Follow up to emberjs#6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
@HeroicEric HeroicEric mentioned this pull request Aug 22, 2019
Merged
HeroicEric added a commit to HeroicEric/data that referenced this pull request Aug 22, 2019
@HeroicEric
[CHORE tests] Replace setupStore for relationship integration tests
831bdff 
Part of emberjs#6166
Follow up to emberjs#6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
HeroicEric added a commit to HeroicEric/data that referenced this pull request Aug 22, 2019
@HeroicEric
[CHORE tests] Replace setupStore for relationship integration tests
e621f26 
Part of emberjs#6166
Follow up to emberjs#6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
runspired pushed a commit that referenced this pull request Aug 22, 2019
@HeroicEric @runspired
[CHORE tests] Replace setupStore for relationship integration tests (#…
c3cfbb9 
…6361)

Part of #6166
Follow up to #6347

This replaces usage of the `setupStore` helper for
`tests/integration/relationships/*`.
@runspired runspired mentioned this pull request Aug 23, 2019
Closed
65 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snewcomer snewcomer snewcomer left review comments

@igorT igorT igorT left review comments

@runspired runspired runspired approved these changes

Assignees
No one assigned
Labels
🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HeroicEric @runspired @igorT @snewcomer