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

caliper-ethereum : Add new tests for connectorFactory and ethereum-connector #1560

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

caliper-ethereum : Add new tests for connectorFactory and ethereum-connector #1560

wants to merge 18 commits into from

Conversation

Sweetdevil144
Copy link

@Sweetdevil144 Sweetdevil144 commented May 1, 2024 •
edited
Loading

Added test cases for caliper-ethereum package
Ready for Review

Checklist

  • A link to the issue/user story that the pull request relates to~ Unit tests for caliper-ethereum #1556
  • 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

  1. cd packages/caliper-ethereum && npm i && npm run dev

Existing issues

Design of the fix

Validation of the fix

Automated Tests

What documentation has been provided for this pull request

@Sweetdevil144 Sweetdevil144 requested a review from a team May 1, 2024 08:09
@Sweetdevil144
Add tests for caliper-ethereum package
b581ee8 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Update test files for ethereum-connector.js
d40236f 
Signed-off-by: Abhinav Pandeey <abhinav.pandey.met22@itbhu.ac.in>

Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Update test cases for ethereum-connector.js
9e010f6 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@davidkel
Copy link
Contributor

davidkel commented May 2, 2024

@aklenik Can you take a look ?

davidkel
davidkel requested changes May 3, 2024
View reviewed changes
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.

Needs some more work I'm afraid but good to see an initial attempt.

@Sweetdevil144
Update connectorFactory test utils
4d3ff42 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
chore: Update connectorFactory test utils
7912cf4 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Test Invalid network connections for connectorFactory.js
7ae2f49 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Update EthereumConnector.js suites
52f90b3 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Update ethereumConenctor.js
3019394 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Copy link
Author

Hey,

Sorry for the inactivity. I've been busy from the past few days due to travel. I'll wrap this up soon.

@Sweetdevil144
Update sample-configs
63c45ff 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Copy link
Author

I've been encountering an error : connection not open. Is this a valid error or is there something going wrong?

@Sweetdevil144
Update checkConfig() tests
7929e14 
Temporarily removed tests for installSmartContracts for debugging purposes

Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
davidkel
davidkel requested changes May 21, 2024
View reviewed changes
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.

Nearly there now for an initial set of tests for this.

packages/caliper-ethereum/test/ethereum-connector.js Outdated Show resolved Hide resolved
packages/caliper-ethereum/test/ethereum-connector.js Outdated Show resolved Hide resolved
packages/caliper-ethereum/test/ethereum-connector.js Outdated Show resolved Hide resolved
@Sweetdevil144
Copy link
Author

Sweetdevil144 commented May 21, 2024 •
edited
Loading

I've been encountering an error : connection not open. Is this a valid error or is there something going wrong?

Any Updates on this?

Suites Prepared. Can you review the suites now?

@Sweetdevil144
Update 'describe' messages and remove empty test suites
40a06d4 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Facing Package Issues while Running test suites
3d27fae 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
@Sweetdevil144
Updated test suites for caliper-ethereun package
b2f4ae1 
Signed-off-by: Abhinav Pandey <abhinav.pandey.met22@itbhu.ac.in>
davidkel
davidkel reviewed May 28, 2024
View reviewed changes
packages/caliper-ethereum/test/ethereum-connector.js
'./sample-configs/invalidUrlConfig.json'
);
ConfigUtil.set(ConfigUtil.keys.NetworkConfig, invalidConfig);
expect(() => new EthereumConnector(invalidConfig)).to.throw(
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be part of the connector factory. As stated before we don't want to have tests that directly test the constructor of the EthereumConnector as it isn't really a public constructor. Please move this test to the ConnectorFactory tests.

davidkel
davidkel reviewed May 28, 2024
View reviewed changes
packages/caliper-ethereum/test/ethereum-connector.js
});

describe('When constructed with absent url path', function () {
it('should throw an error', 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 test should be part of the connector factory. As stated before we don't want to have tests that directly test the constructor of the EthereumConnector as it isn't really a public constructor. Please move this test to the ConnectorFactory tests.

davidkel
davidkel reviewed May 28, 2024
View reviewed changes
packages/caliper-ethereum/test/ethereum-connector.js
ConfigUtil.set(ConfigUtil.keys.NetworkConfig, invalidConfig);
expect(() => new EthereumConnector(invalidConfig)).to.throw(
'No URL given to access the Ethereum SUT. Please check your network configuration. ' +
'Please see https://hyperledger.github.io/caliper/v0.3/ethereum-config/ for more info.'
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is actually a bug. The error message should not provide links like this as it means it needs to be updated with version changes. So we should fix the code and the test. Again this is an example where we should not assume that the code is correct and the test just passes because it is wrong as well. This is a good bug you have found though and simple to fix. We need to change the code to just say to refer to the ethereum configuration in the caliper documentation

davidkel
davidkel requested changes May 28, 2024
View reviewed changes
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.

Nearly there now for this set of tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidkel davidkel davidkel requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Sweetdevil144 @davidkel