add test for the message handling behavior

[tunedev]

Checklist

• A link to the issue/user story that the pull request relates to
• How to recreate the problem without the fix
• Design of the fix
• How to prove that the fix works
• Automated tests that prove the fix keeps on working
• Documentation - any JSDoc, website, or Stackoverflow answers?

Issue/User story

Partially addresses #1606

Steps to Reproduce

Existing issues

• Stack Overflow issues
• GitHub Issues
• Discord history

Design of the fix

Validation of the fix

After this PR: Running the tests in caliper-core the listed %stmts in the code coverage report should tally with the below listed

• caliper-core/lib/worker/worker-message-handler.js: 89.13

Before the PR: Running the tests in caliper-core the listed %stmts in the code coverage report the below listed was what was there before

• caliper-core/lib/worker/worker-message-handler.js: 9.78

Automated Tests

What documentation has been provided for this pull request

[davidkel]

Sorry, changes required here

[tunedev]

Thanks for the feedback, would tend to all of them in order

…

On Wed, 18 Sept 2024 at 09:55, Dave Kelsey ***@***.***> wrote: ***@***.**** requested changes on this pull request. Sorry, changes required here ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + + afterEach(() => { + mockery.deregisterAll(); + mockery.disable(); + sandbox.reset(); + }); + + it('should register the worker when receiving a "register" message', async () => { + new WorkerMessageHandler(messengerMock, connectorFactoryMock); + const message = { + getType: () => MessageTypes.Register, + getSender: () => 'manager-uuid', + stringify: () => 'register message' + }; + + const registerHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Register).args[1]; I don't think we should be finding the registered internal function and invoking it to prove it works. This is like trying to drive a private method. I'm sure there would be better ways to do this. ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + const assignHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.AssignId).args[1]; + await assignHandler(message); + + sinon.assert.calledOnce(messengerMock.send); + sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(AssignedMessage)); + }); + + it('should initialize the worker when receiving an "initialize" message', async () => { + new WorkerMessageHandler(messengerMock, connectorFactoryMock); + connectorFactoryMock.resolves('connector-instance'); + const message = { + getType: () => MessageTypes.Initialize, + stringify: () => 'initialize message' + }; + + const registeredHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Initialize).args[1]; same comment about calling internal methods here. ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + sinon.assert.calledOnce(messengerMock.send); + sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(ReadyMessage)); + }); + + it('should prepare the test when receiving a "prepare" message', async () => { + const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock); + handler.worker = workerMock; + const message = { + getType: () => MessageTypes.Prepare, + getRoundIndex: () => 0, + stringify: () => 'prepare message' + }; + + workerMock.prepareTest.resolves(); + + const registeredHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Prepare).args[1]; same comment about calling internal methods here. ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + + sinon.assert.calledOnce(workerMock.executeRound); + sinon.assert.calledOnce(messengerMock.send); + sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(TestResultMessage)); + }); + + it('should resolve the exit promise when receiving an "exit" message', async () => { + const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock); + handler.exitPromiseFunctions.resolve = sandbox.stub(); + const message = { + getType: () => MessageTypes.Exit, + stringify: () => 'exit message' + }; + + + const exitHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Exit).args[1]; same comment about calling internal methods here. ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + +const sinon = require('sinon'); +const chai = require('chai'); +chai.should(); +const mockery = require('mockery'); + +const MessageTypes = require('../../lib/common/utils/constants').Messages.Types; +const ConnectedMessage = require('../../lib/common/messages/connectedMessage'); +const AssignedMessage = require('../../lib/common/messages/assignedMessage'); +const ReadyMessage = require('../../lib/common/messages/readyMessage'); +const PreparedMessage = require('../../lib/common/messages/preparedMessage'); +const TestResultMessage = require('../../lib/common/messages/testResultMessage'); +const WorkerMessageHandler = require('../../lib/worker/worker-message-handler'); +const CaliperWorker = require('../../lib/worker/caliper-worker'); + +describe('Message Handling Behavior', () => { This describe could be something like "When receiving a message" ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + warnOnUnregistered: false, + useCleanCache: true + }); + + mockery.registerMock('./caliper-worker', { + CaliperWorker: () => workerMock + }); + }); + + afterEach(() => { + mockery.deregisterAll(); + mockery.disable(); + sandbox.reset(); + }); + + it('should register the worker when receiving a "register" message', async () => { "should register the worker when it's a "register" message" ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + it('should register the worker when receiving a "register" message', async () => { + new WorkerMessageHandler(messengerMock, connectorFactoryMock); + const message = { + getType: () => MessageTypes.Register, + getSender: () => 'manager-uuid', + stringify: () => 'register message' + }; + + const registerHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Register).args[1]; + await registerHandler(message); + + sinon.assert.calledOnce(messengerMock.send); + sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(ConnectedMessage)); + }); + + it('should assign the worker ID when receiving an "assignId" message', async () => { see pattern for register message ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + new WorkerMessageHandler(messengerMock, connectorFactoryMock); + const message = { + getType: () => MessageTypes.AssignId, + getWorkerIndex: () => 1, + stringify: () => 'assignId message' + }; + + + const assignHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.AssignId).args[1]; + await assignHandler(message); + + sinon.assert.calledOnce(messengerMock.send); + sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(AssignedMessage)); + }); + + it('should initialize the worker when receiving an "initialize" message', async () => { see pattern for register message ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + getType: () => MessageTypes.Prepare, + getRoundIndex: () => 0, + stringify: () => 'prepare message' + }; + + workerMock.prepareTest.resolves(); + + const registeredHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Prepare).args[1]; + await registeredHandler(message); + + sinon.assert.calledOnce(workerMock.prepareTest); + sinon.assert.calledOnce(messengerMock.send); + sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(PreparedMessage)); + }); + + it('should execute the test round when receiving a "test" message', async () => { see pattern for register message ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + getRoundIndex: () => 0, + stringify: () => 'test message' + }; + + workerMock.executeRound.resolves(); + + const registeredHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Test).args[1]; + await registeredHandler(message); + + + sinon.assert.calledOnce(workerMock.executeRound); + sinon.assert.calledOnce(messengerMock.send); + sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(TestResultMessage)); + }); + + it('should resolve the exit promise when receiving an "exit" message', async () => { see pattern for register message ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + it('should resolve the exit promise when receiving an "exit" message', async () => { + const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock); + handler.exitPromiseFunctions.resolve = sandbox.stub(); + const message = { + getType: () => MessageTypes.Exit, + stringify: () => 'exit message' + }; + + + const exitHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Exit).args[1]; + await exitHandler(message); + + handler.exitPromiseFunctions.resolve.should.have.been.calledOnce; + }); + + it('should throw Error when constructor validations are violated', () => { This should be in it's own describe "When creating a Worker Message Handler" We should also split this into 3 tests because we want to provide details of each failure, the above doesn't tell me what the constructor validations are ------------------------------ In packages/caliper-core/test/worker/worker-message-handler.js <#1626 (comment)>: > + const message = { + getType: () => MessageTypes.Exit, + stringify: () => 'exit message' + }; + + + const exitHandler = messengerMock.on.getCalls().find(call => call.args[0] === MessageTypes.Exit).args[1]; + await exitHandler(message); + + handler.exitPromiseFunctions.resolve.should.have.been.calledOnce; + }); + + it('should throw Error when constructor validations are violated', () => { + const createHandler = (messenger, connectorFactory) => { + return () => new WorkerMessageHandler(messenger, connectorFactory); + }; does it need to be this convoluted ? wouldn't expect(() => { new WorkerMessageHandler(undefined, connectorFactory); }).to.throw(/'Messenger instance is undefined'); work as this is easier to see what exactly was being done — Reply to this email directly, view it on GitHub <#1626 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH6TKS7XKJE2OGQUU2YHA3DZXE5YJAVCNFSM6AAAAABOAHNW3SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMJSGAZDIOBXGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[davidkel]

LGTM