Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Address duplication & missing test cases in registration commands #9043

Conversation

sitetester
Copy link
Contributor

@sitetester sitetester commented Sep 27, 2023

What was the problem?

This PR resolves #8193

How was it solved?

  • Current code throws error 'Validators blsKeys must be unique and lexicographically ordered', which doesn't explicitly tell whether it's unique or lexicographically ordered
  • It addresses duplication in registration commands (& in their test files) by moving common code to parent class
  • Missing tests added

How was it tested?

Tests are functional

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #9043 (2219d68) into release/6.1.0 (5c689a9) will decrease coverage by 0.80%.
The diff coverage is 93.70%.

❗ Current head 2219d68 differs from pull request most recent head 8ab9a36. Consider uploading reports for the commit 8ab9a36 to get more accurate results

Impacted file tree graph

@@                Coverage Diff                @@
##           release/6.1.0    #9043      +/-   ##
=================================================
- Coverage          84.34%   83.54%   -0.80%     
=================================================
  Files                651      604      -47     
  Lines              23985    22697    -1288     
  Branches            3490     3346     -144     
=================================================
- Hits               20229    18962    -1267     
+ Misses              3756     3735      -21     
Files Coverage Δ
...ommander/src/bootstrapping/commands/keys/create.ts 100.00% <100.00%> (ø)
elements/lisk-cryptography/src/encrypt.ts 97.22% <100.00%> (+0.02%) ⬆️
framework/src/application.ts 82.41% <100.00%> (ø)
framework/src/index.ts 100.00% <100.00%> (ø)
framework/src/modules/dynamic_reward/constants.ts 100.00% <ø> (ø)
framework/src/modules/dynamic_reward/endpoint.ts 100.00% <100.00%> (ø)
framework/src/modules/dynamic_reward/index.ts 100.00% <ø> (ø)
framework/src/modules/dynamic_reward/method.ts 71.42% <ø> (ø)
framework/src/modules/dynamic_reward/module.ts 96.55% <100.00%> (ø)
framework/src/modules/dynamic_reward/schemas.ts 100.00% <100.00%> (ø)
... and 13 more

... and 67 files with indirect coverage changes

@sitetester sitetester self-assigned this Sep 27, 2023
@sitetester sitetester marked this pull request as ready for review September 27, 2023 07:58
@sitetester sitetester marked this pull request as draft September 29, 2023 08:48
@sitetester
Copy link
Contributor Author

Need to rebase on release/6.1 branch

@sitetester sitetester force-pushed the 8193-unit-test-review-interop-registration-commands branch from 2219d68 to 8ab9a36 Compare September 29, 2023 10:34
@sitetester sitetester changed the base branch from release/6.0.0 to release/6.1.0 September 29, 2023 10:35
@sitetester sitetester marked this pull request as ready for review September 29, 2023 10:50
@sitetester sitetester changed the base branch from release/6.1.0 to development September 29, 2023 11:51
@sitetester sitetester changed the base branch from development to release/6.1.0 September 29, 2023 11:51
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

I think the PR is deviating from the issue. There is a lot of refactoring and its not required by the issue.

@sitetester sitetester removed the request for review from gkoumout October 10, 2023 11:04
@sitetester sitetester changed the title Unit Test Review: Interoperability registration commands Address duplication in registration commands Oct 10, 2023
@sitetester sitetester closed this Oct 10, 2023
@sitetester sitetester deleted the 8193-unit-test-review-interop-registration-commands branch October 10, 2023 11:24
@sitetester sitetester changed the title Address duplication in registration commands Address duplication & missing test cases in registration commands Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants