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

Improved Common Lib Unit Test Readability #14

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

Conversation

aruokhai
Copy link
Contributor

@aruokhai aruokhai commented Jun 4, 2024

Objective

The objective of this change is to improve the readability of the common lib module unit test, which is necessary for effective testing, given that a peculiar function extractPubKey in the common lib is quite crucial for the correctness of the Project,

Problem

The common lib module is not utilising behavioural test patterns which makes isolating a specific branch of the code under test quite difficult. The problem is quite critical for functions with extreme number of branch paths such as extractPubKey .

Changes

  • Added a test fixtures file in the common lib module, which contains the necessary data need for testing all common lib functions.
  • Added behavioural unit test pattern in the common lib module test file and separated a monolithic test case to branch based test cases.

Scope Of Change

This is a non critical change because it only affects a unit test file.

…t_fixtures file to include all necessary data needed for testing the Common lib functionalities,

converted the Common lib test file to uitise behavioural pattern by splitting a monolithic "extractPubKeyFromScript" test function into sperate branch based test functions

Signed-off-by: Joshua Aruokhai <[email protected]>
Copy link
Collaborator

@theanmolsharma theanmolsharma left a comment

Choose a reason for hiding this comment

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

I don't see any utility in splitting the fixtures by type. I'm all for moving the fixtures to a dedicated file called fixtures.ts but the way you've done it adds more duplicate code to the tests.

Action items:

  • just add one array with all fixtures in a file called fixtures.ts
  • revert splitting the tests into multiple it blocks

@aruokhai
Copy link
Contributor Author

aruokhai commented Jun 9, 2024

I don't see any utility in splitting the fixtures by type. I'm all for moving the fixtures to a dedicated file called fixtures.ts but the way you've done it adds more duplicate code to the tests.

Action items:

  • just add one array with all fixtures in a file called fixtures.ts
  • revert splitting the tests into multiple it blocks

The basic idea of my pattern is to isolate and thoroughly test each code path. I would love it if more explanation is given why it is unnecessary.

@theanmolsharma
Copy link
Collaborator

I don't see any utility in splitting the fixtures by type. I'm all for moving the fixtures to a dedicated file called fixtures.ts but the way you've done it adds more duplicate code to the tests.
Action items:

  • just add one array with all fixtures in a file called fixtures.ts
  • revert splitting the tests into multiple it blocks

The basic idea of my pattern is to isolate and thoroughly test each code path. I would love it if more explanation is given why it is unnecessary.

The isolation of each execution branch comes at the following cost:

  • Code duplication
  • Increased maintenance overhead

For this particular change, the cons outweigh the benefits.

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