Refactor acceptance tests #1103
Conversation
wycats
force-pushed
the
refactor-acceptance-tests
branch
from
fd26634 to
890ccb1
Compare
|
I recommend that you start the review with |
wycats
force-pushed
the
refactor-acceptance-tests
branch
2 times, most recently
from
34a9699 to
5aad4a5
Compare
The main goal is to get the tests to more accurately reflect what actually happen in real applications. Instead of mocking/reopening the application "port", we now directly replace the adapter with a `TestAdapter` that faithfully implements the required hooks the same way that a "real" adapter does. This allows us to write acceptance tests by only sending/responding to messages in a way that is, as a far as the inspector app can is concerned, indistinguishable from the "real" ember debug behaves in the real world. The new API also allows us to be a bit more precise in terms of the expectations (messages sent/received the correct number of times, in the right order, etc).
wycats
force-pushed
the
refactor-acceptance-tests
branch
from
5aad4a5 to
0b541c6
Compare
| if (!this.detectedApplications.mapBy('applicationId').includes(app.applicationId)) { | ||
| this.detectedApplications.push(app); | ||
| } | ||
| message.apps.forEach(({ applicationId, applicationName }) => { |
There was a problem hiding this comment.
Does this not need to do a push into this.detectedApplications? Is it not a list?
There was a problem hiding this comment.
It is not a list anymore, it is a dictionary/object of { applicationId: applicationName } (e.g. { "ember123": "Ember Inspector", "ember456": "Ember Observer" }), so it uses Ember.set to instead
There was a problem hiding this comment.
Ah, I see. So this is setting the application name under the key of application id. Makes sense 👍 .
|
|
||
| selectedDidChange: observer('selectedApp', function() { | ||
| this.port.set('applicationId', this.selectedApp); | ||
| }), |
There was a problem hiding this comment.
This seems unnecessary, since we already have an action here, and on the port side we also have a callback to maintain the applicationId/applicationName when they change.
| @@ -98,7 +98,6 @@ export default Controller.extend({ | |||
| */ | |||
| pinnedObjectId: null, | |||
| inspectingViews: false, | |||
| viewTreeLoaded: false, | |||
There was a problem hiding this comment.
Now unneeded, since the model hook of the route now blocks until the tree is loaded.
| let Adapter; | ||
| if (typeOf(instance.adapter) === 'string') { | ||
| Adapter = instance.resolveRegistration(`adapter:${instance.adapter}`); | ||
| } else { |
There was a problem hiding this comment.
I don't think this branch is possible: on line 9 we unconditionally set it to a string, so seems impossible for it to fall through.
| @@ -19,6 +19,7 @@ export default Route.extend({ | |||
| this.applicationBooted = ({ booted }) => { | |||
| if (booted) { | |||
| port.off('general:applicationBooted', this.applicationBooted); | |||
| this.applicationBooted = null; | |||
There was a problem hiding this comment.
In deactivate it checks for this variable to avoid calling off twice, but we never unset it.
| return new Promise(resolve => { | ||
| this.port.one('view:viewTree', resolve); | ||
| this.port.send('view:getTree'); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Now blocks until we get a tree, so we don't have to worry about the viewTreeLoaded = false state anymore
| this.port.one('deprecation:deprecationsAdded', resolve); | ||
| this.port.send('deprecation:watch'); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
This now blocks until we received the message from ember-debug, which is unconditional (it still sends it even when there are no deprecations).
| * An array of objects of the form: | ||
| * { applicationId, applicationName } | ||
| * A dictionary of the form: | ||
| * { applicationId: applicationName } |
There was a problem hiding this comment.
Keep track of the names as well, in addition to the IDs. Changed from an array to a dictionary object.
| @@ -132,7 +132,6 @@ const EmberDebug = EmberObject.extend({ | |||
| this.startModule('deprecationDebug', DeprecationDebug); | |||
|
|
|||
| this.generalDebug.sendBooted(); | |||
| this.viewDebug.sendTree(); | |||
There was a problem hiding this comment.
This will be requested by the component tree route as needed.
The main goal is to get the tests to more accurately reflect what actually happen in real applications. Instead of mocking/reopening the application "port", we now directly replace the adapter with a
TestAdapterthat faithfully implements the required hooks the same way that a "real" adapter does.This allows us to write acceptance tests by only sending/responding to messages in a way that is, as a far as the inspector app can is concerned, indistinguishable from the "real" ember debug behaves in the real world. The new API also allows us to be a bit more precise in terms of the expectations (messages sent/received the correct number of times, in the right order, etc).
Split from #1088. I'll rebase that PR and merge in these changes when this lands.