Skip to content

refactor(builder): Change HathorManager.__init__ and change tests to use TestBuilder#559

Merged
msbrogli merged 1 commit intodevfrom
tests/use-builder
Apr 20, 2023
Merged

refactor(builder): Change HathorManager.__init__ and change tests to use TestBuilder#559
msbrogli merged 1 commit intodevfrom
tests/use-builder

Conversation

@msbrogli
Copy link
Copy Markdown
Member

@msbrogli msbrogli commented Apr 20, 2023

Acceptance criteria

  1. Make parameters peer_id, tx_storage, and network required on HathorManager.__init__(...).
  2. Create a TestBuilder to build all objects for tests.
  3. Refactor tests to use TestBuilder.

Note: I was considering to refactor CliBuilder to use Builder but I felt it was too much for now. Besides that, I don't need to change the CliBuilder to move forward with my project.

@msbrogli msbrogli added the tests label Apr 20, 2023
@msbrogli msbrogli requested a review from glevco April 20, 2023 06:52
@msbrogli msbrogli requested a review from jansegre as a code owner April 20, 2023 06:52
@msbrogli msbrogli self-assigned this Apr 20, 2023
@msbrogli msbrogli force-pushed the tests/use-builder branch 4 times, most recently from 2cfdfe4 to da73891 Compare April 20, 2023 07:07
@msbrogli msbrogli changed the title test(builder): Change tests to use TestBuilder refactor(builder): Change HathorManager.__init__ and change tests to use TestBuilder Apr 20, 2023
@msbrogli msbrogli force-pushed the tests/use-builder branch 3 times, most recently from 2d6c65c to 232319f Compare April 20, 2023 07:24
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 20, 2023

Codecov Report

Merging #559 (1f84f43) into dev (f7ef8a7) will increase coverage by 0.04%.
The diff coverage is 91.91%.

❗ Current head 1f84f43 differs from pull request most recent head 7b2dd3c. Consider uploading reports for the commit 7b2dd3c to get more accurate results

@@            Coverage Diff             @@
##              dev     #559      +/-   ##
==========================================
+ Coverage   83.30%   83.35%   +0.04%     
==========================================
  Files         212      214       +2     
  Lines       18729    18959     +230     
  Branches     2596     2614      +18     
==========================================
+ Hits        15603    15803     +200     
- Misses       2561     2582      +21     
- Partials      565      574       +9     
Impacted Files Coverage Δ
hathor/simulator/simulator.py 92.61% <ø> (ø)
hathor/builder/builder.py 91.70% <91.70%> (ø)
hathor/builder/__init__.py 100.00% <100.00%> (ø)
hathor/builder/cli_builder.py 79.19% <100.00%> (ø)
hathor/manager.py 70.73% <100.00%> (-0.38%) ⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Copy Markdown
Member

@jansegre jansegre left a comment

Choose a reason for hiding this comment

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

Functionally I'm more than OK with this PR, I really like the direction we're going with the Builder. I only have some smaller naming and behavior suggestions.

@msbrogli msbrogli force-pushed the tests/use-builder branch 3 times, most recently from f3ed01a to ed092c6 Compare April 20, 2023 14:03
Copy link
Copy Markdown
Contributor

@glevco glevco left a comment

Choose a reason for hiding this comment

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

Like Jan, I like this direction and I only have minor comments. Moving CliBuilder to this solution soon would be great.

@msbrogli msbrogli force-pushed the tests/use-builder branch 3 times, most recently from 1f84f43 to 21148fa Compare April 20, 2023 16:46
@msbrogli msbrogli force-pushed the tests/use-builder branch from 21148fa to 7b2dd3c Compare April 20, 2023 16:47
@msbrogli msbrogli merged commit 7b2dd3c into dev Apr 20, 2023
@msbrogli msbrogli deleted the tests/use-builder branch April 20, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants