Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add more test for recordRate rate-controller #1627

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add more test for recordRate rate-controller #1627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/caliper-core/lib/worker/rate-control/recordRate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ class RecordRateController extends RateInterface {
*/
async applyRateControl() {
await this.recordedRateController.applyRateControl();
this.records[this.stats.getTotalSubmittedTx()] = Date.now() - this.stats.getRoundStartTime();
this.records[this.stats.getTotalSubmittedTx() - 1] = Date.now() - this.stats.getRoundStartTime();
}

/**
Expand Down
318 changes: 275 additions & 43 deletions packages/caliper-core/test/worker/rate-control/recordRate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,22 @@
const mockery = require('mockery');
const path = require('path');
const RecordRate = require('../../../lib/worker/rate-control/recordRate');
const fs = require('fs');
const TestMessage = require('../../../lib/common/messages/testMessage');
const MockRate = require('./mockRate');
const TransactionStatisticsCollector = require('../../../lib/common/core/transaction-statistics-collector');
const util = require('../../../lib/common/utils/caliper-utils');
const logger = util.getLogger('record-rate-controller');

const chai = require('chai');
chai.should();
const sinon = require('sinon');

describe('RecordRate controller', () => {
let msgContent;
let stubStatsCollector;
let sandbox;

before(() => {
mockery.enable({
warnOnReplace: false,
Expand All @@ -34,25 +41,29 @@ describe('RecordRate controller', () => {
});

mockery.registerMock(path.join(__dirname, '../../../lib/worker/rate-control/noRate.js'), MockRate);
sandbox = sinon.createSandbox();
});

after(() => {
mockery.deregisterAll();
mockery.disable();
if (fs.existsSync('../tx_records_client0_round0.txt')) {
fs.unlinkSync('../tx_records_client0_round0.txt');
}
});

it('should apply rate control to the recorded rate controller', async () => {
const msgContent = {
beforeEach(() => {
msgContent = {
label: 'test',
rateControl: {
"type": "record-rate",
"opts": {
"rateController": {
"type": "zero-rate"
type: 'record-rate',
opts: {
rateController: {
type: 'zero-rate'
},
"pathTemplate": "../tx_records_client<C>_round<R>.txt",
"outputFormat": "TEXT",
"logEnd": true
pathTemplate: '../tx_records_client<C>_round<R>.txt',
outputFormat: 'TEXT',
logEnd: true
}
},
workload: {
Expand All @@ -63,42 +74,263 @@ describe('RecordRate controller', () => {
totalWorkers: 2
};

const testMessage = new TestMessage('test', [], msgContent);
const stubStatsCollector = sinon.createStubInstance(TransactionStatisticsCollector);
const rateController = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
const mockRate = MockRate.createRateController();
mockRate.reset();
mockRate.isApplyRateControlCalled().should.equal(false);
await rateController.applyRateControl();
mockRate.isApplyRateControlCalled().should.equal(true);
stubStatsCollector = new TransactionStatisticsCollector();
stubStatsCollector.getTotalSubmittedTx = sandbox.stub();
});

it('should throw an error if the rate controller to record is unknown', async () => {
const msgContent = {
label: 'test',
rateControl: {
"type": "record-rate",
"opts": {
"rateController": {
"type": "nonexistent-rate"
},
"pathTemplate": "../tx_records_client<C>_round<R>.txt",
"outputFormat": "TEXT",
"logEnd": true
}
},
workload: {
module: 'module.js'
},
testRound: 0,
txDuration: 250,
totalWorkers: 2
};
const testMessage = new TestMessage('test', [], msgContent);
afterEach(() => {
sandbox.restore();
});

describe('Export Formats', () => {
it('should default outputFormat to TEXT if undefined', () => {
msgContent.rateControl.opts.outputFormat = undefined;
const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
controller.outputFormat.should.equal('TEXT');
});


it('should set outputFormat to TEXT if invalid format is provided', () => {
msgContent.rateControl.opts.outputFormat = 'INVALID_FORMAT';
const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);

controller.outputFormat.should.equal('TEXT');
});

it('should export records to text format', async () => {
const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
sinon.stub(controller.recordedRateController, 'end').resolves();

controller.records = [100, 200, 300];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better set of data here, might be to do the followint
controller.records[1]=100;
controller.records[3]=200;
controller.records[7]=300;

We would then expect the output to be
0
100
0
200
0
0
0
300

But in fact it won't do this. So there is a bug.
why 0 instead of blank lines ? well I think 0 makes more sense given that the data should be consistent and it's also what would happen if there were a known number of transactions before hand

In fact there are at least 3 bugs in the rate controller (2 in the code and 1 bug/clarification in the documentation) which me looking at this test highlights. We need to make sure that tests being written try to mimic the real world scenarios , so here the assumption that records are going to be sequential in position 0,1,2 are not accurate.

This test does test the format of the output, so we could have another test that tests the real world behaviour but rather than having 2 tests doing almost identical testing we can have one test to do it all. I think both approaches would be ok, but it highlights that this test is not adequate on it's own and again is an example where 100% coverage doesn't mean it's properly tested.

const fsWriteSyncStub = sandbox.stub(fs, 'writeFileSync');
const fsAppendSyncStub = sandbox.stub(fs, 'appendFileSync');

await controller.end();

sinon.assert.calledOnce(fsWriteSyncStub);
sinon.assert.calledThrice(fsAppendSyncStub);

// Verify the content written to the file
sinon.assert.calledWith(fsWriteSyncStub, sinon.match.string, '', 'utf-8');
sinon.assert.calledWith(fsAppendSyncStub.getCall(0), sinon.match.string, '100\n');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to use the controller.records array content and length to drive these checks as it's the content of the records we want to make sure matches.

sinon.assert.calledWith(fsAppendSyncStub.getCall(1), sinon.match.string, '200\n');
sinon.assert.calledWith(fsAppendSyncStub.getCall(2), sinon.match.string, '300\n');


fsWriteSyncStub.restore();
fsAppendSyncStub.restore();
});

it('should export records to binary big endian format', async () => {
const msgContent = {
label: 'test',
rateControl: {
type: 'record-rate',
opts: {
rateController: {
type: 'zero-rate'
},
pathTemplate: '../tx_records_client<C>_round<R>.txt',
outputFormat: 'BIN_BE'
}
},
testRound: 0, // Ensure roundIndex is set
txDuration: 250,
totalWorkers: 2
};
const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
sinon.stub(controller.recordedRateController, 'end').resolves();

controller.records = [100, 200, 300];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about the test data here as for the text test.

const fsWriteSyncStub = sandbox.stub(fs, 'writeFileSync');

await controller.end();

sinon.assert.calledOnce(fsWriteSyncStub);
const buffer = fsWriteSyncStub.getCall(0).args[1];
buffer.readUInt32BE(0).should.equal(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to use the controller.records array content and length to drive these checks as it's the content of the records we want to make sure matches.

buffer.readUInt32BE(4).should.equal(100);
buffer.readUInt32BE(8).should.equal(200);
buffer.readUInt32BE(12).should.equal(300);

fsWriteSyncStub.restore();
});

it('should export records to binary little endian format', async () => {
msgContent.rateControl.opts.outputFormat = 'BIN_LE';
const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);

sandbox.stub(controller.recordedRateController, 'end').resolves();

controller.records = [100, 200, 300];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here regarding the record data given in the text test

const fsWriteSyncStub = sandbox.stub(fs, 'writeFileSync');

await controller.end();

sinon.assert.calledOnce(fsWriteSyncStub);
const buffer = fsWriteSyncStub.getCall(0).args[1];
buffer.readUInt32LE(0).should.equal(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to use the controller.records array content and length to drive these checks as it's the content of the records we want to make sure matches.

buffer.readUInt32LE(4).should.equal(100);
buffer.readUInt32LE(8).should.equal(200);
buffer.readUInt32LE(12).should.equal(300);

fsWriteSyncStub.restore();
});
it('should export to text format when output format is TEXT', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already tested above. given that you track the writes to the file, we know it's only writing to text and not the others.

const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);

const mockController = {
end: sinon.stub().resolves(),
applyRateControl: sinon.stub().resolves(),
};
controller.recordedRateController.controller = mockController;

const exportToTextSpy = sinon.spy(controller, '_exportToText');
const exportToBinaryLittleEndianSpy = sinon.spy(controller, '_exportToBinaryLittleEndian');
const exportToBinaryBigEndianSpy = sinon.spy(controller, '_exportToBinaryBigEndian');

await controller.end();

sinon.assert.calledOnce(exportToTextSpy);
sinon.assert.notCalled(exportToBinaryLittleEndianSpy);
sinon.assert.notCalled(exportToBinaryBigEndianSpy);
sinon.assert.notCalled(logger.error);

exportToTextSpy.restore();
exportToBinaryLittleEndianSpy.restore();
exportToBinaryBigEndianSpy.restore();
});

it('should export to binary little endian format when output format is BIN_LE', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this test as it's already tested.

msgContent.rateControl.opts.outputFormat = 'BIN_LE';
const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);

const mockController = {
end: sinon.stub().resolves(),
applyRateControl: sinon.stub().resolves(),
};
controller.recordedRateController.controller = mockController;

const exportToTextSpy = sinon.spy(controller, '_exportToText');
const exportToBinaryLittleEndianSpy = sinon.spy(controller, '_exportToBinaryLittleEndian');
const exportToBinaryBigEndianSpy = sinon.spy(controller, '_exportToBinaryBigEndian');

await controller.end();

sinon.assert.notCalled(exportToTextSpy);
sinon.assert.calledOnce(exportToBinaryLittleEndianSpy);
sinon.assert.notCalled(exportToBinaryBigEndianSpy);
sinon.assert.notCalled(logger.error);

exportToTextSpy.restore();
exportToBinaryLittleEndianSpy.restore();
exportToBinaryBigEndianSpy.restore();
});


it('should export to binary big endian format when output format is BIN_BE', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this test as it's already tested

msgContent.rateControl.opts.outputFormat = 'BIN_BE';
const testMessage = new TestMessage('test', [], msgContent);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);

const mockController = {
end: sinon.stub().resolves(),
applyRateControl: sinon.stub().resolves(),
};
controller.recordedRateController.controller = mockController;

const exportToTextSpy = sinon.spy(controller, '_exportToText');
const exportToBinaryLittleEndianSpy = sinon.spy(controller, '_exportToBinaryLittleEndian');
const exportToBinaryBigEndianSpy = sinon.spy(controller, '_exportToBinaryBigEndian');

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();
});

it('should throw an error if pathTemplate is undefined', () => {
msgContent.rateControl.opts.pathTemplate = undefined;
const testMessage = new TestMessage('test', [], msgContent);

(() => {
RecordRate.createRateController(testMessage, stubStatsCollector, 0);
}).should.throw('The path to save the recording to is undefined');
});
});

describe('When Applying Rate Control', () => {
it('should apply rate control to the recorded rate controller', async () => {
const testMessage = new TestMessage('test', [], msgContent);
const rateController = RecordRate.createRateController(testMessage, stubStatsCollector, 0);
const mockRate = MockRate.createRateController();
mockRate.reset();
mockRate.isApplyRateControlCalled().should.equal(false);
await rateController.applyRateControl();
mockRate.isApplyRateControlCalled().should.equal(true);
});

it('should replace path template placeholders for various worker and round indices', () => {
const testCases = [
{ testRound: 0, workerIndex: 0, expectedPath: '../tx_records_client0_round0.txt' },
{ testRound: 1, workerIndex: 2, expectedPath: '../tx_records_client2_round1.txt' },
{ testRound: 5, workerIndex: 3, expectedPath: '../tx_records_client3_round5.txt' },
{ testRound: 10, workerIndex: 7, expectedPath: '../tx_records_client7_round10.txt' },
];

testCases.forEach(({ testRound, workerIndex, expectedPath }) => {
const content = JSON.parse(JSON.stringify(msgContent));
content.testRound = testRound;
const testMessage = new TestMessage('test', [], content);
const controller = RecordRate.createRateController(testMessage, stubStatsCollector, workerIndex);
controller.pathTemplate.should.equal(util.resolvePath(expectedPath));
});
});

it('should throw an error if the rate controller to record is unknown', async () => {
msgContent.rateControl.opts.rateController.type = 'nonexistent-rate';
msgContent.rateControl.opts.logEnd = true;
const testMessage = new TestMessage('test', [], msgContent);

(() => {
RecordRate.createRateController(testMessage, stubStatsCollector, 0);
}).should.throw(/Module "nonexistent-rate" could not be loaded/);
});

it('should throw an error if rateController is undefined', () => {
msgContent.rateControl.opts.rateController = undefined;
const testMessage = new TestMessage('test', [], msgContent);

(() => {
RecordRate.createRateController(testMessage, stubStatsCollector, 0);
}).should.throw('The rate controller to record is undefined');
});
});

describe('When Creating a RecordRate Controller', () => {
it('should initialize records array if the number of transactions is provided', () => {
const testMessage = new TestMessage('test', [], msgContent);
sinon.stub(testMessage, 'getNumberOfTxs').returns(5);

const controller = RecordRate.createRateController(testMessage, stubStatsCollector, 0);

controller.records.should.be.an('array').that.has.lengthOf(5);
stubStatsCollector.getTotalSubmittedTx.returns(1);
controller.records.every(record => record.should.equal(0));
});

const stubStatsCollector = sinon.createStubInstance(TransactionStatisticsCollector);
(() => {
RecordRate.createRateController(testMessage, stubStatsCollector, 0)
}).should.throw(/Module "nonexistent-rate" could not be loaded/);
});
});
Loading