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 more tests for caliper worker #1625

Merged

Conversation

tunedev
Copy link
Contributor

@tunedev tunedev commented Sep 10, 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

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 listed %stmts in the code coverage report should tally with the below listed

  • caliper-core/lib/worker/worker-message-handler.js: 89.13

Before the PR: Running the tests in caliper-core the listed %stmts in the code coverage report the below listed was what was there before

  • caliper-core/lib/worker/worker-message-handler.js: 9.78

Automated Tests

What documentation has been provided for this pull request

@tunedev tunedev requested a review from a team September 10, 2024 14:34
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.

Sorry I think this requires a bit of rework. Please don't fall into the trap that you are writing tests that should pass the current implementation only. The approach should be to write tests of the behaviour you expect from the code and then make sure it passes which could result in a needed code change. These tests should find bugs as well as ensure it's easy to make changes and to check the behaviour remains the same

packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
@tunedev tunedev force-pushed the add-test-for-worker-message-handler branch from 486e0b3 to d4a42ab Compare September 11, 2024 06:40
@davidkel davidkel changed the title Add test for worker message handler Add more tests for caliper worker Sep 14, 2024
@tunedev tunedev force-pushed the add-test-for-worker-message-handler branch 2 times, most recently from c99470a to 36a2cb2 Compare September 23, 2024 05:47
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.

Apologies I missed the fact that the worker stops on the first error so we don't need to make the changes to the error handling

packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
@tunedev tunedev force-pushed the add-test-for-worker-message-handler branch from 36a2cb2 to a278abf Compare September 30, 2024 10:50
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.

Still some issues I'm afraid

packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
@tunedev tunedev force-pushed the add-test-for-worker-message-handler branch from a278abf to e20dfcc Compare October 8, 2024 15:10
@tunedev tunedev requested a review from davidkel October 8, 2024 18:42
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 minor things plus ideally we could use some more tests but at least this is more than before

packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
@tunedev tunedev force-pushed the add-test-for-worker-message-handler branch from e20dfcc to e678fb2 Compare October 16, 2024 10:50
@tunedev tunedev requested a review from davidkel October 16, 2024 12:35
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.

some final minor comments.

packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/lib/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
packages/caliper-core/test/worker/caliper-worker.js Outdated Show resolved Hide resolved
Signed-off-by: Babatunde Sanusi <swisskid95@gmail.com>
@tunedev tunedev force-pushed the add-test-for-worker-message-handler branch from e678fb2 to a50f342 Compare October 20, 2024 11:24
@tunedev tunedev requested a review from davidkel October 20, 2024 11:26
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.

Thanks @tunedev

@davidkel davidkel merged commit 334dd2a into hyperledger-caliper:main Oct 22, 2024
17 checks passed
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.

2 participants