-
Notifications
You must be signed in to change notification settings - Fork 405
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.
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
Add fixed-load rate controller to calliper generator #1592
Conversation
Signed-off-by: Vivek jha <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1592 +/- ##
==========================================
+ Coverage 56.01% 56.04% +0.02%
==========================================
Files 105 105
Lines 4508 4511 +3
Branches 691 691
==========================================
+ Hits 2525 2528 +3
Misses 1426 1426
Partials 557 557
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@duckling69 Hi thanks for the contribution, please could you add unit tests to please otherwise we can't accept the PR and as you can see the code coverage has reduced with the addition of the new code. |
Signed-off-by: Vivek jha <[email protected]>
@davidkel did the review changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also include this as an option when running the yo generator. see around lines 101-106 of index.js to update
@@ -272,6 +272,10 @@ module.exports = class extends Generator { | |||
answersObject.opts = 'tps: 100, unfinished_per_client: 100'; | |||
this._configWrite(); | |||
break; | |||
case 'fixed-load': | |||
answersObject.opts = 'startTps:10, transactionLoad:20'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a single space added between property and value so that it's consistent with the others, ie
startTps: 10, transactionLoad: 20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Vivek jha <[email protected]>
@@ -104,6 +104,7 @@ module.exports = class extends Generator { | |||
{name: 'Fixed Backlog', value: 'fixed-backlog'}, | |||
{name: 'Linear Rate', value: 'linear-rate'}, | |||
{name: 'Fixed Feedback Rate', value: 'fixed-feedback-rate'} | |||
{name: 'Fixed Load', value: "fixed-load"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run the tests locally before submitting a PR. Doing so will quickly catch bugs in your submission. Here the build fails because you put a bug into the code, you are missing a ,
at the end of line 106
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the bug in index.js
Signed-off-by: Vivek jha <[email protected]>
Signed-off-by: Vivek jha <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, many thanks for the contribution
fixes #1587