Add tests for rate-control/compositeRate.js

[Sweetdevil144]

This PR aims to create unit-tests for compositeRate.js class within our caliper-core package.

However, the current tests are failing due to following reasons.

Within our packages/caliper-core/lib/worker/rate-control/compositeRate.js, we find the following line :

https://github.com/hyperledger/caliper/blob/21a98f496c850840c211a670c32fcfa9240612bb/packages/caliper-core/lib/worker/rate-control/compositeRate.js#L76

However, I tried logging out this.options and was returned with an empty Object as output.

console.log('Options: ', this.options);
let weights = this.options.weights;
let rateControllers = this.options.rateControllers;

I was returned with following logs :

Options:  {}
2024.05.12-00:43:27.856 error [caliper] [composite-rate-controller]     Weight and controller definitions must be arrays.
      2) should throw error when weights and rateControllers lengths are not the same
(Similar errors for other tests too)

Due to this our initial check at line 86, that is :

https://github.com/hyperledger/caliper/blob/21a98f496c850840c211a670c32fcfa9240612bb/packages/caliper-core/lib/worker/rate-control/compositeRate.js#L79

Always throws an error. However, if we replace the line 76 above with

let weights = this.testMessage.content.weights;

Then, our tests pass.

What would be the most optimum fix for such cases?

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

Steps to Reproduce

Existing issues

• Stack Overflow issues
• GitHub Issues
• Discord history

This issue is a small part of #1557 which aims to increase overall code-coverage for the caliper-core package.

[Sweetdevil144]

Considering above changes have been applied for proper data-handling within _prepareControllers, we get a new error causing recursive functional callings as follow :

 at CompositeRateController._prepareControllers (lib/worker/rate-control/compositeRate.js:173:37)
      at new CompositeRateController (lib/worker/rate-control/compositeRate.js:69:14)
      at createRateController (lib/worker/rate-control/compositeRate.js:275:12)
      at new RateControl (lib/worker/rate-control/rateControl.js:59:27)
      at CompositeRateController._prepareControllers (lib/worker/rate-control/compositeRate.js:173:37)
      at new CompositeRateController (lib/worker/rate-control/compositeRate.js:69:14)
      at createRateController (lib/worker/rate-control/compositeRate.js:275:12)
      at Context.<anonymous> (test/worker/rate-control/compositeRate.js:49:35)
      at process.processImmediate (node:internal/timers:478:21)

[Sweetdevil144]

@davidkel can you help me with this in any way possible?

[davidkel]

@Sweetdevil144 Unfortunately your change is not correct. The reason you are getting an error and options is an empty object is because that's what you have set it to in your test when you created a mock message.

            rateControl: {
                type: 'composite-rate',
                opts: {},

[Sweetdevil144]

@Sweetdevil144 Unfortunately your change is not correct. The reason you are getting an error and options is an empty object is because that's what you have set it to in your test when you created a mock message.

            rateControl: {
                type: 'composite-rate',
                opts: {},

Thanks. The test cases work fine now. However, I found a bug within our packages/caliper-core/lib/worker/rate-control/compositeRate.js. Can you confirm whether I'm correct or now?

I notices that we had been extracting Weights and rateControllers via following configurations :

        let weights = this.options.weights;
        let rateControllers = this.options.rateControllers;

Whereas, it should have been as below :

        let weights = this.testMessage.content.weights;
        let rateControllers = this.testMessage.content.rateControllers;

Can you confirm whether I'm correct or not @davidkel ? I'm pushing the overall applied changes for now. I'll refract them if you feel these aren't corresponding to any internal bug in system.

[davidkel]

@Sweetdevil144 Sorry, no your change is not correct. Not sure how you have come to this conclusion but if you look in the documentation https://hyperledger.github.io/caliper/v0.6.0/rate-controllers/#composite-rate you will see the format of the rate controller spec. weights and rateControllers are properties of opts and opts is extracted into this.options in the constructor see https://github.com/hyperledger/caliper/blob/56ecafa073ddb751375f3357200f84b4264ee13e/packages/caliper-core/lib/worker/rate-control/rateInterface.js#L33

[Sweetdevil144]

@davidkel fixed. Can you review it?

[davidkel]

What we want to try to replicate is the pattern used for writing unit tests as seen in the caliper-fabric package. For example you will see we don't use any method names in the describe or it descriptions, no internal methods are explicitly tested.
Unfortunately the patterns used in other parts of Caliper are from the old way of thinking but it would be a lot of work to go back and refactor the existing tests, but we want to try to make sure that all new tests are done in the new style.

[davidkel]

We don't want to be directly testing internal methods (ie ones that start with _). All tests should drive the public apis.

[Sweetdevil144]

Should we incorporate _prepareControllers within applyRateControl?

[davidkel]

It's not necessarily about incorporating especially if the internal method is called from multiple public methods. You need to write tests that make sense for the public methods and if the internal method gets called multiple times by the tests then that's fine. As an example a public method which has multiple code flows but to test all those code flows you may end up always calling the private method which is expected.

[davidkel]

We don't want to be directly testing internal methods (ie ones that start with _). All tests should drive the public apis.