Refactor API #713
Refactor API #713
Conversation
|
By analyzing the blame information on this pull request, we identified @sotojuan, @naptowncode and @ariporad to be potential reviewers |
| this.hasExclusive = true; | ||
| this.testCount = 0; | ||
| } | ||
| testData.listenToTestRun(emitter); |
Yea that's always stuck me as strange. Note though that the reporters reference these properties. Their tests use a stubbed |
|
|
I set reporter.api to the new testData instance with each I added a to-do comment that that should be cleaned up. I might just change it to pass |
👍 |
jamestalmage
force-pushed
the
simplify-api
branch
3 times, most recently
from
62cd74e to
112fa8a
Compare
jamestalmage
force-pushed
the
simplify-api
branch
from
112fa8a to
bdce76a
Compare
| @@ -431,7 +447,8 @@ group('chokidar is installed', function (beforeEach, test, group) { | |||
| t.plan(2); | |||
|
|
|||
| files = ['foo-{bar,baz}.js']; | |||
| api.excludePatterns = ['!*bar*']; | |||
| // TODO(@jamestalmage, @novemberborn): There is no way for users to actually set exclude patterns yet. | |||
| avaFiles.defaultExcludePatterns.returns(['!*bar*']); | |||
There was a problem hiding this comment.
@novemberborn - Currently, there is no way for users to set the exclude patterns. Is this a feature that needs to be exposed?
You access api.excludePatterns in watcher, but that's a static value that never changes (I guess an API consumer could, but we haven't published that, so ...).
Should we open an issue about this, or is avaFiles.defaultExcludePatterns() all you need.
There was a problem hiding this comment.
@jamestalmage you could add exclude patterns to files. We should just make sure to still use the default include patterns if files only contains negated patterns. That's how the watcher behaves with source patterns.
| var EventEmitter = require('events').EventEmitter; | ||
| var formatter = require('./enhance-assert').formatter(); | ||
|
|
||
| function TestData(opts) { |
There was a problem hiding this comment.
@jamestalmage did you mean to rename this to RunStatus?
Since #713 the API no longer emits 'dependencies' events. These are emitted instead by the RunStatus, which can be obtained by listening to the 'test-run' event on the API. The watcher tests use a mocked API object which wasn't updated to reflect these changes. Consequently dependency tracking was broken since #713 was merged. This commit fixes the watcher and the corresponding tests. I've also added an integration test which does not rely on mocking, helping us detect breakage sooner.
Since #713 the API no longer emits 'dependencies' events. These are emitted instead by the RunStatus, which can be obtained by listening to the 'test-run' event on the API. The watcher tests use a mocked API object which wasn't updated to reflect these changes. Consequently dependency tracking was broken since #713 was merged. This commit fixes the watcher and the corresponding tests. I've also added an integration test which does not rely on mocking, helping us detect breakage sooner.
This is preliminary work on simplifying the API. The major (breaking) change is a new
test-runevent that passes an emitter for that specific test run. Said emitter is an instance of the new TestData class (might be more appropriately named TestRun). This contains test results that we were storing directly on the API instance. That really doesn't make sense anymore given that the API instance is called multiple times by Watcher. I feel this way makes a little more sense. There is definitely room for further refactoring, but this represents a big diff, and I want to get it reviewed and merged before further refactoring.