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.

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 test for caliper-engine module #1638

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

Conversation

tunedev
Copy link
Contributor

@tunedev tunedev commented Oct 8, 2024

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

Closes #1637

Steps to Reproduce

Existing issues

Design of the fix

Validation of the fix

Automated Tests

What documentation has been provided for this pull request

@tunedev tunedev requested a review from a team as a code owner October 8, 2024 15:43
Signed-off-by: Babatunde Sanusi <[email protected]>
@tunedev tunedev force-pushed the add-test-to-manager-caliper-engine-module branch from 05f794e to bf0217f Compare October 8, 2024 15:48
Copy link
Contributor

@davidkel davidkel left a comment

Choose a reason for hiding this comment

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

A few tweaks required

// TODO: Implement test
});

it('should handle cases where the round orchestrator is not running', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This text should be explicit about what it is testing. So in this case please use
should do nothing if no benchmark run has been started

Whether a benchmark run has been started can be tested in 2 ways

  1. the run method has not been called
  2. the run method has been called but performTest was not set to true

});

describe('Stop Method Functionality', function() {
it('should stop the round orchestrator if it is running', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can be more explicit here
should stop the benchmark if the benchmark has been started

this test should find a bug

});

context('When end commands are to be executed', function() {
it('should execute the end command successfully', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

end commands are always run if the flag is set to true, even if an error occurs on any of the other stages. This scenario needs to be tested

});
});

describe('Run Method Execution Flow', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Still don't like putting implementation details in the test so please change this. This section is about when a benchmark is run

});
});

describe('Stop Method Functionality', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I don't like to see implementation details. This section is about when a request to stop a benchmark is made

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test to the caliper-engine module
2 participants