Closed gabrielcsapo closed 4 years ago
This is more or less how we ended cancellation to be used... thanks @gabrielcsapo for digging in. I remember doing a bunch of prep work to enable this, I didn't realize I got so far that this was the only step remaining. I'm trying to recall if there was something else which prevented me from doing this last step.... It is very possible I simply ran out of time.
I think the reason I didn't complete this at the time may have been related to ember-cli not yet using broccoli's built in watcher (which I believe it is now), it was still using the broccoli-sane-watcher. And I wanted a good end2end manual test in a real app.
@gabrielcsapo I'm assuming you have tested ^^ in your app at work? (That was the same app I wanted to test this with). Let me know the outcome of this test, if it is/was successful I feel good about moving forward
Ya, I agree with @krisselden. The mocking in the tests here make it pretty hard to tell if the changes work in reality...
@rwjblue @krisselden I would be more than happy to fix, if you have time would love to peer and figure out how to make these tests more robust.
@gabrielcsapo I agree, mock's and spies are often anti-patterns in testing.
For cancellation the following pattern works: create a test with a multi step pipeline, give the test control to complete each step in the pipeline, cancel during 1 step, and assert the next step is not run. It should be relatively easy to adapt this pattern to trigger the cancelation via an input change.
@stefanpenner using your suggestion I have implemented a test case without using spies, mocks or stubs.
released as v3.4.0 🎉
FWIW, this is causing issues downstream (see https://github.com/ember-cli/ember-cli/issues/9127). I think we should probably revert until we have done an bit more investigation...
I will look into this, revert makes the most sense 😢
https://github.com/ember-cli/ember-cli/blob/master/lib/models/builder.js#L169 so it looks like the ember-cli builder code catches any errors thrown by broccoli. Cancellation errors make sense as those should be ignored.
@stefanpenner do we need to throw on cancellation? or should this simply return?
async build() {
if (this._cancelationRequest) {
throw new BuilderError('Cannot start a build if one is already running');
}
}
could be turned into
async build() {
if (this._cancelationRequest) { return; }
}
Looking at this code, should we throw when things are cancelled? Cancellations are expected especially in this case.
I'm working on a fix
fixes #407
taken from @rwjblue