TESTING: Revisit testing of computed property dependent keys

[jonathandavidson]

I feel that the recommended method of testing dependent keys for computed properties is problematic for several reasons:

1. Relying on the _dependentKeys property is problematic since there is no guarantee it will exist in future Ember versions and in which case all of these tests would need to be rewritten.
2. The specificity of the tests provides little value since all a failing test tells us is that a dependent key was removed, in which case the developer would have removed the corresponding test anyway.
3. Testing these directly in lieu of testing the resulting functionality provided by the dependent key being present has a high potential to leave gaps in our testing.

As an alternative, I feel that the emphasis should be on testing units of functionality rather than directly testing that specific properties are set. For example, if the desired functionality of a component is that a triggered action changes the value of a property on the component, the test should trigger the action then test that the value is what is expected. This way the component could be refactored and the tests still pass. If, on the other hand, the tests are too specific, they would all fail after a refactor and need to be rewritten. Then we have to ask ourselves why we are even writing tests in the first place.

[notmessenger]

1. This is a valid point, though when such a change would occur it would be during an Ember upgrade and a quick search/replace could resolve it. On its own I do not believe this point carries enough weight to be the deciding factor but it should be considered.
2. The removal of the corresponding test may appear to be a result of the removal of a dependent key but that is only from the perspective of final changes being code reviewed. What happens during the refactoring of a method which causes a key to be removed and is discovered during the running of the test suites? The test added value.
3. The dependent keys are not (to be) tested in lieu of testing the resulting functionality provided by the dependent keys being present. See the 4th sub-point of the first point of the Component Unit Tests guide:

the logic of the computed property functions

Regarding your suggested alternative, no emphasis has been removed from testing units of functionality. In order to test such things, one could reasonably make a call to a method, say myTestMethod() for example, and test that known input results in expected output. The inner workings of this method could be refactored with the expectation that this test would still pass. This is for sure a version of refactoring. Another version is where argument signatures or returns are changed. Yet another is where property and method names are changed. In these cases tests are going to fail no matter what, because of the nature of the refactor. Your suggestion only views things from the perspective of the first type of refactoring, while ignoring the other two. I am not saying what you are suggesting isn't valid - in fact the testing guides say to do what you are suggesting. What I'm saying is that there are other factors to consider as well, which the current approach does.

[jonathandavidson]

Some counterpoints:

1. All other things being equal, I'd take the approach that requires less work, even if it is just a quick search and replace. And that's assuming it even would be that trivial.
2. If we are testing the required functionality, those tests will fail if the removal of a dependent key breaks that functionality. If it doesn't break that functionality they will still pass meaning that the dependent key is no longer a necessity. On the contrary, the test testing the dependent key specifically will fail either way whether the dependent key was really needed or not. It doesn't really tell us anything. It's just extra work to remove that test (And to add any others if there are new dependent keys).
3. "the logic of the computed property functions" - Perhaps we can add some clarification around this. I took it to mean we call componentInstance.get( 'myComputedProperty' ) to be sure it returns what is expected. We definitely want to do that as well as testing that other things happening in the component cause this property to be updated. If we are doing this already then it just reinforces point number 2 above.