Refactor methods in viewer into performance-impl #6252
Conversation
muxin
force-pushed
the
refactor-performance-in-viewer
branch
from
a312009
to
750557f
Compare
| @@ -257,6 +257,7 @@ var forbiddenTerms = { | |||
| whitelist: [ | |||
| 'src/service/viewer-impl.js', | |||
| 'src/service/storage-impl.js', | |||
| 'src/service/performance-impl.js', | |||
There was a problem hiding this comment.
@erwinmombay I saw we have whitelist rule for sendMessage but not the sendMessageCancelUnsent (previously sendMessageUnreliable). Do we need to add rules for sendMessageCancelUnsent too?
| }); | ||
| }); | ||
|
|
||
| it('should ignore all calls to tick', () => { | ||
| perf.tick('start0'); | ||
| return perf.coreServicesAvailable().then(() => { | ||
| expect(tickSpy.callCount).to.equal(0); | ||
| expect(viewerSendMessageStub.withArgs('tick').callCount).to.equal(0); |
| }); | ||
| }); | ||
|
|
||
| it('should ignore all calls to flush', () => { | ||
| perf.tick('start0'); | ||
| perf.flush(); | ||
| return perf.coreServicesAvailable().then(() => { | ||
| expect(flushTicksSpy.callCount).to.equal(0); | ||
| expect(viewerSendMessageCancelUnsentStub.withArgs('sendCsi') |
| @@ -221,7 +221,7 @@ export class Performance { | |||
| * @private | |||
| */ | |||
| setFlushParams_(params) { | |||
| this.viewer_.setFlushParams(params); | |||
| this.viewer_.sendMessageCancelUnsent('setFlushParams', params, false); | |||
There was a problem hiding this comment.
Can we make the false optional?
There was a problem hiding this comment.
If we make it optional - we need to change the default value to true.
There was a problem hiding this comment.
We can make the default value to false, since most of the case is not waiting for response
There was a problem hiding this comment.
Might be worth doing a quick math. But I think most important calls actually do wait on response. We had a couple of annoying errors related to this. E.g.
return promise1.then(() => {
return viewer.sendMessage('message', payload);
}).then(result => {
});
The expectation here is that it'll block on the message response. In actuality this block will not block and instead immediately yield.
This error was so bad that I made third argument to be required. I wonder if we should just split methods to sendMessage and sendMessageNoResult. This would remove the ugly Promise|undefined response type and make types more complete.
/cc @lannka
There was a problem hiding this comment.
I argued for getting rid of the parameter entirely. Always return the promise, cause we generate it anyways.
There was a problem hiding this comment.
@dvoytenko 2 methods make sense. sendMessage and sendMessageAwaitResponse.
@jridgewell that's different, we still need to pass true|false to viewer to inform whether a response is expected. Simply removing the param at our side would not solve the problem @dvoytenko has mentioned.
There was a problem hiding this comment.
We can set it to true for everything?
There was a problem hiding this comment.
I believe the boolean was introduced to reduce the unncessary response postMessage from search viewer. On the other hand, I'm also not sure if search viewer can handle true for every message.
There was a problem hiding this comment.
I'm going to create another PR for the refactoring of awaitResponse.
I'm not sure whether the viewer is respecting the true/false value set by us or just having predefined behavior depending on the eventType (postMessage for certain type of event without checking the awaitResponse param)
| label, | ||
| from: opt_from, | ||
| value: opt_value, | ||
| }); | ||
| }, false); |
There was a problem hiding this comment.
This argument should be annotated everywhere.
| */ | ||
| flush() { | ||
| if (this.isMessagingReady_ && this.isPerformanceTrackingOn_) { | ||
| this.viewer_.flushTicks(); | ||
| this.viewer_.sendMessageCancelUnsent('sendCsi', undefined, false); |
There was a problem hiding this comment.
Ditto, const arguments need annotating.
muxin
force-pushed
the
refactor-performance-in-viewer
branch
from
750557f
to
d5603ab
Compare
| // coreServicesAvailable calls flush once. | ||
| return perf.coreServicesAvailable().then(() => { | ||
| expect(flushTicksSpy.callCount).to.equal(1); | ||
| expect(viewerSendMessageCancelUnsentStub.withArgs('sendCsi') | ||
| .callCount).to.equal(1); |
There was a problem hiding this comment.
since we're in the process of making the test nicer too, can you try out the spy.should.have.callCount(n) syntax? https://github.com/domenic/sinon-chai
There was a problem hiding this comment.
Do you mean expect(spy).to.have.callCount(n)? I see that we are using expect everywhere, not spy.should.have....
There was a problem hiding this comment.
would this work? expect(viewerSendMessageCancelUnsentStub.withArgs('sendCsi')).to.be.calledOnce
There was a problem hiding this comment.
@lannka Later in this test we have callCount = 2, callCount = 3 ... It seems we still need to assert the callCount
muxin
force-pushed
the
refactor-performance-in-viewer
branch
from
d5603ab
to
8e5478c
Compare
|
PTAL |
|
@dvoytenko PTAL |
Part of #6159