Skip to content

Speed up fail rest tests#267

Merged
stdevMac merged 12 commits intoNethermindEth:mainfrom
v4lproik:speed-up-fail-rest-tests
Jul 5, 2022
Merged

Speed up fail rest tests#267
stdevMac merged 12 commits intoNethermindEth:mainfrom
v4lproik:speed-up-fail-rest-tests

Conversation

@v4lproik
Copy link
Copy Markdown
Contributor

@v4lproik v4lproik commented Jul 4, 2022

Resolves #256

Changes:

  • Extracted the hardcoded retry mechanism implemented in the http Do function.
  • Allows the http client to take a retry mechanism as parameter which will then be used in the Do function.
  • Test time before this PR
ok  	github.com/NethermindEth/juno/pkg/rest/tests	123.929s
  • Test time after this PR
ok  	github.com/NethermindEth/juno/pkg/rest/tests	5.299s

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • [] No

In case you checked yes, did you write tests??

  • Yes
  • No

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 4, 2022

Codecov Report

Merging #267 (e1ffe6b) into main (64ec930) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #267   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           65        65           
  Lines         5907      5919   +12     
=========================================
+ Hits          5907      5919   +12     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/feeder/feeder.go 100.00% <100.00%> (ø)
pkg/rest/rest.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64ec930...e1ffe6b. Read the comment docs.

@stdevMac
Copy link
Copy Markdown
Member

stdevMac commented Jul 4, 2022

Just some coverage issues, you can take a look deeply if you look at here. Very well done work! Thanks for your contributions!

@v4lproik
Copy link
Copy Markdown
Contributor Author

v4lproik commented Jul 4, 2022

I added a test that fails when using the default client implementation and so by extension the default retry mechanism. Coverage should be ok now.

Test time is now at ok github.com/NethermindEth/juno/pkg/rest/tests 18.872s

Thanks for your message. It looks like an exciting project. I'll definitely contribute more.

Copy link
Copy Markdown
Contributor

@tshakalekholoane tshakalekholoane left a comment

Choose a reason for hiding this comment

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

See comments. Otherwise great work! We appreciate your contribution.

Comment thread pkg/rest/tests/rest_test.go Outdated
Comment thread pkg/rest/tests/rest_test.go Outdated
Comment thread pkg/rest/tests/rest_test.go Outdated
Comment thread pkg/rest/tests/rest_test.go Outdated
Comment thread pkg/rest/tests/rest_test.go Outdated
Comment thread pkg/rest/tests/rest_test.go Outdated
@v4lproik
Copy link
Copy Markdown
Contributor Author

v4lproik commented Jul 5, 2022

Thanks for the review @tshakalekholoane. I also ended up extracting query url and fixing the error formatting in the handlers.

@tshakalekholoane
Copy link
Copy Markdown
Contributor

Awesome, @v4lproik! Just update the baseApi variable as well to baseAPI and we should be set.

@stdevMac stdevMac merged commit 8e4cfd2 into NethermindEth:main Jul 5, 2022
@v4lproik v4lproik deleted the speed-up-fail-rest-tests branch July 5, 2022 20:06
IronGauntlets pushed a commit that referenced this pull request Aug 18, 2022
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.

Reduce overall testing time of juno through changes in REST tests

4 participants