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

rl/e2e configure orchestrator #2443

Merged
merged 13 commits into from
Jun 23, 2022
Merged

rl/e2e configure orchestrator #2443

merged 13 commits into from
Jun 23, 2022

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Jun 1, 2022

What does this pull request do? Explain your changes. (required)
E2E test orchestrator configuration

Specific updates (required)

  • Start a geth node
  • Start go-livepeer node in orchestrator mode
  • Register orchestrator
  • Wait for the next round
  • Reconfigure orchestrator
  • Check updated values

How did you test each of these updates (required)
Tests with ./test.sh and ./test_e2e.sh pass

Does this pull request close any open issues?
Fixes #2421

Checklist:

@red-0ne red-0ne force-pushed the rl/e2e-configure-orchestrator branch from 9c48519 to fc79445 Compare June 1, 2022 13:47
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #2443 (f1c239a) into master (2e9387e) will decrease coverage by 0.03218%.
The diff coverage is 50.00000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2443         +/-   ##
===================================================
- Coverage   55.05601%   55.02383%   -0.03219%     
===================================================
  Files             94          94                 
  Lines          19729       19726          -3     
===================================================
- Hits           10862       10854          -8     
- Misses          8265        8270          +5     
  Partials         602         602                 
Impacted Files Coverage Δ
cmd/livepeer/starter/starter.go 2.33236% <0.00000%> (-0.00454%) ⬇️
server/webserver.go 95.69892% <100.00000%> (-0.21945%) ⬇️
discovery/db_discovery.go 67.01389% <0.00000%> (-1.04167%) ⬇️

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 2e9387e...f1c239a. Read the comment docs.

@red-0ne red-0ne marked this pull request as ready for review June 14, 2022 13:43
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Redouane. I added a few comments. Other than that:

  1. Could you update the Checklist in the PR description (either check the box or cross out the whole point if it's not related)
  2. There is something weird about why your test takes so much time to execute and why you had to split it into 2 tests.

I tried to do all the required steps manually using devtool and livepeer_cli. And doing them manually took only 1.5 min. So, I guess your test should take no more than 1.5 min. I recored a movie of the steps. Could you please check and try it yourself and then try to see what's wrong with the test you wrote?

2022-06-15.12-44-32.mp4

server/handlers.go Outdated Show resolved Hide resolved
server/handlers_test.go Outdated Show resolved Hide resolved
server/webserver.go Outdated Show resolved Hide resolved
test/e2e/configure_orchestrator_test.go Outdated Show resolved Hide resolved
test/e2e/configure_orchestrator_test.go Outdated Show resolved Hide resolved
test/e2e/configure_orchestrator_test.go Outdated Show resolved Hide resolved
test/e2e/configure_orchestrator_test.go Show resolved Hide resolved
test/e2e/remove_orchestrator_test.go Outdated Show resolved Hide resolved
test/e2e/remove_orchestrator_test.go Outdated Show resolved Hide resolved
test/e2e/remove_orchestrator_test.go Show resolved Hide resolved
@red-0ne
Copy link
Contributor Author

red-0ne commented Jun 15, 2022

Could you please check and try it yourself and then try to see what's wrong with the test you wrote?

I think I know what's happening. It's actually when changing blockRewardCut and feeShare that are on-chain operations that require waiting for rounds. I guess if you try to trigger option 13. Set orchestrator config after being registered and activated, this will cause the error I faced. Will double check it and update here.

@red-0ne red-0ne requested a review from leszko June 21, 2022 02:22
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Thanks @red-0ne for all the work. Added some more comments. They are mainly about:

  1. Removing the time-consuming withdraw stake part
  2. Some restructure of the tests for the sake of readability

Other than that, the tests are fine. I'm happy we have them :)

cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
server/handlers.go Outdated Show resolved Hide resolved
server/handlers_test.go Show resolved Hide resolved
server/webserver.go Outdated Show resolved Hide resolved
test/e2e/e2e.go Outdated Show resolved Hide resolved
test/e2e/e2e.go Outdated Show resolved Hide resolved
test/e2e/e2e.go Show resolved Hide resolved
test/e2e/remove_orchestrator_test.go Outdated Show resolved Hide resolved
test/e2e/remove_orchestrator_test.go Outdated Show resolved Hide resolved
@red-0ne red-0ne force-pushed the rl/e2e-configure-orchestrator branch from fbb56c0 to 6ca67be Compare June 22, 2022 10:55
@red-0ne red-0ne requested a review from leszko June 22, 2022 18:48
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added one comment. Other than that LGTM 👍

Thanks for the good work @red-0ne

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
Co-authored-by: Rafał Leszko <[email protected]>
@red-0ne red-0ne merged commit 00a3e6d into master Jun 23, 2022
@hjpotter92 hjpotter92 deleted the rl/e2e-configure-orchestrator branch September 26, 2022 17:00
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.

E2E Test scenario: Configure Orchestrator
2 participants