Test untested caliper core worker modules #1607
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tunedev
force-pushed
the
test-untested-caliper-core_worker-modules
branch
3 times, most recently
from
fdf4a4c to
1e3cdf4
Compare
davidkel
requested changes
Aug 23, 2024
|
Thanks for your feedback, would start addressing them ASAP
…On Fri, 23 Aug 2024 at 10:59, Dave Kelsey ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hi sorry, some changes are required. I see you weren't aware about calling
private methods in tests and realised it is something we should add to the
contributing unit tests in the contributing section. So would be good to
add something there.
------------------------------
In packages/caliper-core/lib/worker/caliper-worker.js
<#1607 (comment)>:
> @@ -110,13 +110,14 @@ class CaliperWorker {
/**
* Perform test with specified test duration
* @param {object} workloadModule The user test module.
- * @param {Object} duration duration to run for
+ * @param {Object} duration duration to run forASc
This looks like a typo
------------------------------
In packages/caliper-core/test/worker/rate-control/recordRate.js
<#1607 (comment)>:
> - "logEnd": true
- }
- },
- workload: {
- module: 'module.js'
- },
- testRound: 0,
- txDuration: 250,
- totalWorkers: 2
- };
- const testMessage = new TestMessage('test', [], msgContent);
+ afterEach(() => {
+ sandbox.restore();
+ });
+
+ describe('happy path', () => {
Sorry I don't like this description in the describe. The it statement
provide details on what the final outcome should be. This describe block
should collate the tests under a common group of activities. I'm more than
happy for multiple describes to split this up and also that these describes
can contain error scenarios as well. For example, the testing of the export
formats can be done under their own describe. It may also make sense not to
have some of the tests under a describe. See the fabric tests for examples.
------------------------------
In packages/caliper-core/test/worker/rate-control/recordRate.js
<#1607 (comment)>:
> +
+ await controller.end();
+
+ sinon.assert.notCalled(exportToTextSpy);
+ sinon.assert.notCalled(exportToBinaryLittleEndianSpy);
+ sinon.assert.calledOnce(exportToBinaryBigEndianSpy);
+ sinon.assert.notCalled(logger.error);
+
+ exportToTextSpy.restore();
+ exportToBinaryLittleEndianSpy.restore();
+ exportToBinaryBigEndianSpy.restore();
+ });
+
+ });
+
+ describe('Error path', () => {
See previous comment
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (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('WorkerMessageHandler', () => {
Describes should not just be a Class name, we want to avoid that. In the
fabric tests we use a descriptive name for the Class
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> + warnOnUnregistered: false,
+ useCleanCache: true
+ });
+
+ mockery.registerMock('./caliper-worker', {
+ CaliperWorker: () => workerMock
+ });
+ });
+
+ afterEach(() => {
+ mockery.deregisterAll();
+ mockery.disable();
+ sandbox.reset();
+ });
+
+ it('should handle "register" message', async () => {
We need to describe here the behaviour, just saying handle doesn't provide
enough information of the behaviour being tested.
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> +
+ it('should handle "register" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ const message = {
+ getType: () => MessageTypes.Register,
+ getSender: () => 'manager-uuid',
+ stringify: () => 'register message'
+ };
+
+ await handler._handleRegisterMessage(message);
+
+ sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(ConnectedMessage));
+ });
+
+ it('should handle "assignId" message', async () => {
We need to describe here the behaviour, just saying handle doesn't provide
enough information of the behaviour being tested.
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> + it('should handle "initialize" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ connectorFactoryMock.resolves('connector-instance');
+ const message = {
+ getType: () => MessageTypes.Initialize,
+ stringify: () => 'initialize message'
+ };
+
+ await handler._handleInitializeMessage(message);
+
+ sinon.assert.calledOnce(connectorFactoryMock);
+ sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(ReadyMessage));
+ });
+
+ it('should handle "prepare" message', async () => {
We need to describe here the behaviour, just saying handle doesn't provide
enough information of the behaviour being tested.
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> +
+ it('should handle "assignId" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ const message = {
+ getType: () => MessageTypes.AssignId,
+ getWorkerIndex: () => 1,
+ stringify: () => 'assignId message'
+ };
+
+ await handler._handleAssignIdMessage(message);
+
+ sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(AssignedMessage));
+ });
+
+ it('should handle "initialize" message', async () => {
We need to describe here the behaviour, just saying handle doesn't provide
enough information of the behaviour being tested.
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> + const message = {
+ getType: () => MessageTypes.Prepare,
+ getRoundIndex: () => 0,
+ stringify: () => 'prepare message'
+ };
+
+ workerMock.prepareTest.resolves();
+
+ await handler._handlePrepareMessage(message);
+
+ sinon.assert.calledOnce(workerMock.prepareTest);
+ sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(PreparedMessage));
+ });
+
+ it('should handle "test" message', async () => {
We need to describe here the behaviour, just saying handle doesn't provide
enough information of the behaviour being tested.
------------------------------
In packages/caliper-core/test/worker/caliper-worker.js
<#1607 (comment)>:
> @@ -161,5 +161,84 @@ describe('Caliper worker', () => {
await worker.executeRound(mockTestMessage);
validateCallsAndWarnings(4);
});
+
+ it('should wait for all transactions to finish', async () => {
+ const mockRoundStats = {
+ getTotalFinishedTx: sinon.stub(),
+ getTotalSubmittedTx: sinon.stub()
+ };
+
+ mockRoundStats.getTotalFinishedTx.onCall(0).returns(0);
+ mockRoundStats.getTotalFinishedTx.onCall(1).returns(1);
+ mockRoundStats.getTotalSubmittedTx.returns(1);
+
+ await CaliperWorker._waitForTxsToFinish(mockRoundStats);
We really want to avoid writing unit tests for private methods (ie methods
that have the _ in fromt of them) we look to drive the testing at a public
method level only. The only place in caliper where it looks like this is
being broken is on the _sendSingleRequest method. However this is actually
a method that has to be implemented by a connector and is called by
something external so is infact a public method just not labelled as such.
------------------------------
In packages/caliper-core/test/worker/rate-control/recordRate.js
<#1607 (comment)>:
> + const testMessage = new TestMessage('test', [], msgContent);
+ const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
+
+ controller.pathTemplate.should.equal(util.resolvePath('../tx_records_client0_round0.txt'));
+ });
+
+ it('should export records to text format', () => {
+ const msgContent = { ...defaultMsgContent };
+ const testMessage = new TestMessage('test', [], msgContent);
+ const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
+
+ controller.records = [100, 200, 300];
+ const fsWriteSyncStub = sandbox.stub(fs, 'writeFileSync');
+ const fsAppendSyncStub = sandbox.stub(fs, 'appendFileSync');
+
+ controller._exportToText();
see comment about calling private methods in tests
------------------------------
In packages/caliper-core/test/worker/rate-control/recordRate.js
<#1607 (comment)>:
> + buffer.readUInt32BE(8).should.equal(200);
+ buffer.readUInt32BE(12).should.equal(300);
+
+ fsWriteSyncStub.restore();
+ });
+
+ it('should export records to binary little endian format', () => {
+ const msgContent = { ...defaultMsgContent };
+ msgContent.rateControl.opts.outputFormat = 'BIN_LE';
+ const testMessage = new TestMessage('test', [], msgContent);
+ const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
+
+ controller.records = [100, 200, 300];
+ const fsWriteSyncStub = sandbox.stub(fs, 'writeFileSync');
+
+ controller._exportToBinaryLittleEndian();
see comment about calling private methods in tests
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> +
+ afterEach(() => {
+ mockery.deregisterAll();
+ mockery.disable();
+ sandbox.reset();
+ });
+
+ it('should handle "register" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ const message = {
+ getType: () => MessageTypes.Register,
+ getSender: () => 'manager-uuid',
+ stringify: () => 'register message'
+ };
+
+ await handler._handleRegisterMessage(message);
see comment about calling private methods in tests
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> +
+ await handler._handleRegisterMessage(message);
+
+ sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(ConnectedMessage));
+ });
+
+ it('should handle "assignId" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ const message = {
+ getType: () => MessageTypes.AssignId,
+ getWorkerIndex: () => 1,
+ stringify: () => 'assignId message'
+ };
+
+ await handler._handleAssignIdMessage(message);
see comment about calling private methods in tests
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> +
+ await handler._handleAssignIdMessage(message);
+
+ sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(AssignedMessage));
+ });
+
+ it('should handle "initialize" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ connectorFactoryMock.resolves('connector-instance');
+ const message = {
+ getType: () => MessageTypes.Initialize,
+ stringify: () => 'initialize message'
+ };
+
+ await handler._handleInitializeMessage(message);
see comment about calling private methods in tests
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> + sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(ReadyMessage));
+ });
+
+ it('should handle "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();
+
+ await handler._handlePrepareMessage(message);
see comment about calling private methods in tests
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> + sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(PreparedMessage));
+ });
+
+ it('should handle "test" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ handler.worker = workerMock;
+ const message = {
+ getType: () => MessageTypes.Test,
+ getRoundIndex: () => 0,
+ stringify: () => 'test message'
+ };
+
+ workerMock.executeRound.resolves();
+
+ await handler._handleTestMessage(message);
see comment about calling private methods in tests
------------------------------
In packages/caliper-core/test/worker/worker-message-handler.js
<#1607 (comment)>:
> + await handler._handleTestMessage(message);
+
+ sinon.assert.calledOnce(workerMock.executeRound);
+ sinon.assert.calledOnce(messengerMock.send);
+ sinon.assert.calledWith(messengerMock.send, sinon.match.instanceOf(TestResultMessage));
+ });
+
+ it('should handle "exit" message', async () => {
+ const handler = new WorkerMessageHandler(messengerMock, connectorFactoryMock);
+ handler.exitPromiseFunctions.resolve = sandbox.stub();
+ const message = {
+ getType: () => MessageTypes.Exit,
+ stringify: () => 'exit message'
+ };
+
+ await handler._handleExitMessage(message);
see comment about calling private methods in tests
—
Reply to this email directly, view it on GitHub
<#1607 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH6TKS4JFN6HCVYYOHEH7NLZS4BZJAVCNFSM6AAAAABMXOOLRCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENJWG44TCNZRGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
caliper-worker worker-message-handler fixedFeedbackRate recordRate Signed-off-by: Babatunde Sanusi <[email protected]>
tunedev
force-pushed
the
test-untested-caliper-core_worker-modules
branch
from
1e3cdf4 to
d0270f4
Compare
aklenik
added
the
mentorship-2024-testing
davidkel
requested changes
Sep 10, 2024
|
You mean to separate the PRS into individually tested modules right.
…On Tue, Sep 10, 2024, 12:45 PM Dave Kelsey ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Could you merge these changes into the individual PRs as requested in your
other PR please. It will make it easier to review for us and also hopefully
make it easier to get code merged quicker
—
Reply to this email directly, view it on GitHub
<#1607 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH6TKS2EWCB2JMTC7ECDAQLZV3LVXAVCNFSM6AAAAABMXOOLRCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOJSGE4TCNBUG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
Issue/User story
Partially addresses #1606
Steps to Reproduce
Existing issues
Design of the fix
Validation of the fix
After this PR: Running the tests in caliper-core the below listed %stmts in the code coverage report should tally with the below listed
Before the PR: Running the tests in caliper-core the below listed %stmts in the code coverage report the below listed was what was there before
Automated Tests
What documentation has been provided for this pull request