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

Add test asserting no unnecessary inverse work #4247

Merged
merged 1 commit into from
Mar 21, 2016

Conversation

asakusuma
Copy link
Contributor

Ensures that if inverse: null, inverseFor is not called.

Test for #4225. Addresses #4236.

cc @krisselden @stefanpenner @pangratz

@asakusuma
Copy link
Contributor Author

The test fails without the original PR commit. Still need to add a test to ensure inverse searching is not done if a specific relationship is indicated for the inverse.

@stefanpenner
Copy link
Member

This test needs to assert the final state is as expected, the test as-is would pass if the input data was merely incorrect.

@asakusuma
Copy link
Contributor Author

I feel like a "final state" test should be separate from the test added in this PR. The final state and avoiding unnecessary work are two distinct concerns/expectations.

Additionally, the relationship integration tests already assert final state of relationships where inverse is explicitly set to null. Is there some additional specific concern that we need to test here?

@stefanpenner
Copy link
Member

sorta, so, this only tests that an explicit method for handling inverses is NOT called, but that isn't refactoring friendly. A more robust test would also include a scenario where the both the method is called and isn't called. That way the test would fail, if the mechanism of calculating inverses changed was refactored out. Which would hopefully force the refactorer to consider this scenario.

My suggestion should be a small addition to your existing tests, and would provide a high confidence regression test.

@asakusuma
Copy link
Contributor Author

Ah, that makes sense. I'll update the PR.

@stefanpenner
Copy link
Member

@asakusuma sorry, I was having a hard time trying to convey what i meant here Thanks for your patience.

@asakusuma
Copy link
Contributor Author

@stefanpenner no problem, thanks for the guidance. I've updated the tests accordingly.

@krisselden @pangratz it's going to take some reworking in order to test that inverse resolution work is not done when inverse is explicitly set. findPossibleInverses is a local function and hence not exposed for spying. I believe this should be tackled in a separate PR.

EDIT: link to findPossibleInverses:

//If inverse is specified manually, return the inverse
if (options.inverse) {
inverseName = options.inverse;
inverse = Ember.get(inverseType, 'relationshipsByName').get(inverseName);
assert("We found no inverse relationships by the name of '" + inverseName + "' on the '" + inverseType.modelName +
"' model. This is most likely due to a missing attribute on your model definition.", !Ember.isNone(inverse));
inverseKind = inverse.kind;
} else {
//No inverse was specified manually, we need to use a heuristic to guess one
if (propertyMeta.type === propertyMeta.parentType.modelName) {
warn(`Detected a reflexive relationship by the name of '${name}' without an inverse option. Look at http://emberjs.com/guides/models/defining-models/#toc_reflexive-relation for how to explicitly specify inverses.`, false, {
id: 'ds.model.reflexive-relationship-without-inverse'
});
}
var possibleRelationships = findPossibleInverses(this, inverseType);

@stefanpenner
Copy link
Member

@asakusuma the updated tests look wonderful, thank you :)

krisselden added a commit that referenced this pull request Mar 21, 2016
Add test asserting no unnecessary inverse work
@krisselden krisselden merged commit fbde070 into emberjs:master Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants