-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
UI: Gracefully handle stat errors #4833
Changes from 5 commits
022573a
9f98029
b0a1a36
333e76b
c95821e
b5348aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import { copy } from '@ember/object/internals'; | ||
|
||
// Used with fetch. | ||
// Fetch only goes into the promise catch if there is a network error. | ||
// This means that handling a 4xx or 5xx error is the responsibility | ||
// of the developer. | ||
const jsonWithDefault = defaultResponse => res => | ||
res.ok ? res.json() : copy(defaultResponse, true); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nice, I just realized it depends on the Isn't that a https://developer.mozilla.org/en-US/docs/Web/API/Response/ok So maybe The commenting here helps, but if I'm in a file that uses this I might think that this just requires a json blob, whereas it needs a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I considered this interface gotcha, but decided ultimately that I was probably overthinking it, and it isn't as big of a footgun as it seems under a microscope. If it comes up again, I'll considering moving it. |
||
export default jsonWithDefault; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
import { resolve } from 'rsvp'; | ||
import wait from 'ember-test-helpers/wait'; | ||
import { test } from 'ember-qunit'; | ||
import sinon from 'sinon'; | ||
|
||
const MockResponse = json => ({ | ||
ok: true, | ||
json() { | ||
return resolve(json); | ||
}, | ||
}); | ||
|
||
export default function statsTrackerFrameMissing({ | ||
resourceName, | ||
TrackerConstructor, | ||
ResourceConstructor, | ||
mockFrame, | ||
compileResources, | ||
}) { | ||
test('a bad response from a fetch request is handled gracefully', function(assert) { | ||
const frame = mockFrame(1); | ||
const [compiledCPU, compiledMemory] = compileResources(frame); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know you generally don't like lingering comments, so thought I'd check you'd noticed this, not a biggie for me especially as its in tests, maybe it helps to help folks understand what's happening here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yep! This should be deleted. This code is now expected of the caller of the behavior. |
||
let shouldFail = false; | ||
const fetch = () => { | ||
return resolve(shouldFail ? { ok: false } : new MockResponse(frame)); | ||
}; | ||
|
||
const resource = ResourceConstructor(); | ||
const tracker = TrackerConstructor.create({ fetch, [resourceName]: resource }); | ||
|
||
tracker.get('poll').perform(); | ||
|
||
return wait() | ||
.then(() => { | ||
assert.deepEqual(tracker.get('cpu'), [compiledCPU], 'One frame of cpu'); | ||
assert.deepEqual(tracker.get('memory'), [compiledMemory], 'One frame of memory'); | ||
|
||
shouldFail = true; | ||
tracker.get('poll').perform(); | ||
return wait(); | ||
}) | ||
.then(() => { | ||
assert.deepEqual(tracker.get('cpu'), [compiledCPU], 'Still one frame of cpu'); | ||
assert.deepEqual(tracker.get('memory'), [compiledMemory], 'Still one frame of memory'); | ||
assert.equal(tracker.get('frameMisses'), 1, 'Frame miss is tracked'); | ||
|
||
shouldFail = false; | ||
tracker.get('poll').perform(); | ||
return wait(); | ||
}) | ||
.then(() => { | ||
assert.deepEqual(tracker.get('cpu'), [compiledCPU, compiledCPU], 'Still one frame of cpu'); | ||
assert.deepEqual( | ||
tracker.get('memory'), | ||
[compiledMemory, compiledMemory], | ||
'Still one frame of memory' | ||
); | ||
assert.equal(tracker.get('frameMisses'), 0, 'Frame misses is reset'); | ||
}); | ||
}); | ||
|
||
test('enough bad responses from fetch consecutively (as set by maxFrameMisses) results in a pause', function(assert) { | ||
const fetch = () => { | ||
return resolve({ ok: false }); | ||
}; | ||
|
||
const resource = ResourceConstructor(); | ||
const tracker = TrackerConstructor.create({ | ||
fetch, | ||
[resourceName]: resource, | ||
maxFrameMisses: 3, | ||
pause: sinon.spy(), | ||
}); | ||
|
||
tracker.get('poll').perform(); | ||
return wait() | ||
.then(() => { | ||
assert.equal(tracker.get('frameMisses'), 1, 'Tick misses'); | ||
assert.notOk(tracker.pause.called, 'Pause not called yet'); | ||
|
||
tracker.get('poll').perform(); | ||
return wait(); | ||
}) | ||
.then(() => { | ||
assert.equal(tracker.get('frameMisses'), 2, 'Tick misses'); | ||
assert.notOk(tracker.pause.called, 'Pause still not called yet'); | ||
|
||
tracker.get('poll').perform(); | ||
return wait(); | ||
}) | ||
.then(() => { | ||
assert.equal(tracker.get('frameMisses'), 0, 'Misses reset'); | ||
assert.ok(tracker.pause.called, 'Pause called now that frameMisses == maxFrameMisses'); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More ember-data adapter like things that normally you might use ember-data for (see my comment from earlier to do on another PR). Actually now I've seen this again, did you say somewhere there was a decision made not to use ember-data for this? Does the same go for the non-ember-data usage in
system
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I wrote out the rationale on the PR that introduced stat trackers.
And yes, similar rationale for things like leader and regions. They're one-off calls that aren't going to change, have no relationships, have no computed properties, aren't CRUD, etc. If that changes some day, I'll convert them into ED models.