Add foundation for E2E Tests + First E2E Scenario: Register Orchestrator #2383

leszko · 2022-04-29T11:18:41Z

What does this pull request do? Explain your changes. (required)
Create first E2E Scenario which registers an orchestrator using local chain.

How to run the test:

You need to have Docker running
(Mac M1) Update Image in e2e.go to darkdragon/geth-with-livepeer-protocol:streamflow
Fire ./test_e2e.sh

For more details about next steps, please check E2E Tests Epic

Specific updates (required)

Extract devtool package from devtool.go to make the devtool code reusable
Extract starter package from livepeer.go to make the livepeer code reusable
Create e2e.go with the reusable utils for E2E Tests
Create register_orchestrator_test.go with the first E2E Test scenario
Create/Update test.sh/test_e2e.sh

How did you test each of these updates (required)

./test-e2e.sh
check if nothing is broken with devtool
check if nothing is broken with livepeer

Does this pull request close any open issues?
fix #2317

Checklist:

Read the contribution guide
make runs successfully
All tests in ./test.sh pass
~~README and other documentation updated~~
Pending changelog updated

codecov · 2022-04-29T11:25:05Z

Codecov Report

Merging #2383 (fa4bdbc) into master (4d80d57) will increase coverage by 0.47864%.
The diff coverage is 2.37624%.

@@                 Coverage Diff                 @@
##              master       #2383         +/-   ##
===================================================
+ Coverage   54.50763%   54.98627%   +0.47863%     
===================================================
  Files             93          93                 
  Lines          19467       19303        -164     
===================================================
+ Hits           10611       10614          +3     
+ Misses          8267        8100        -167     
  Partials         589         589

Impacted Files	Coverage Δ
cmd/livepeer/starter/starter.go	`2.37624% <2.37624%> (ø)`
cmd/livepeer/livepeer.go
discovery/db_discovery.go	`65.97222% <0.00000%> (+1.04165%)`	⬆️

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 4d80d57...fa4bdbc. Read the comment docs.

thomshutt · 2022-04-29T16:27:59Z

test/e2e/e2e.go

+		ContainerRequest: req,
+		Started:          true,
+	})
+	if err != nil {


Can trim the code down a bit with require.NoError(t, err) rather than this conditional pattern

Fixed in 6064454

thomshutt · 2022-04-29T16:34:11Z

If the devtool_utils are only going to be used in the tests then could we move them over to some more specific files/packages within the E2E code?

thomshutt · 2022-04-29T16:40:19Z

Not specific to this PR, but we should look at moving the M1 image to a livepeer-owned repo

thomshutt · 2022-05-04T09:07:28Z

I tried running the tests (M1 MacOS. I updated the image name to the darkdragon one) and it seems to hang after this point:

++ go list ./...
++ grep test/e2e
+ go test github.com/livepeer/go-livepeer/test/e2e
# github.com/karalabe/usb
In file included from ../../../../pkg/mod/github.com/karalabe/[email protected]/libs.go:50:
../../../../pkg/mod/github.com/karalabe/[email protected]/libusb/libusb/os/darwin_usb.c:253:39: warning: 'kIOMasterPortDefault' is deprecated: first deprecated in macOS 12.0 [-Wdeprecated-declarations]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/IOKit.framework/Headers/IOKitLib.h:123:19: note: 'kIOMasterPortDefault' has been explicitly marked deprecated here
In file included from ../../../../pkg/mod/github.com/karalabe/[email protected]/libs.go:50:
../../../../pkg/mod/github.com/karalabe/[email protected]/libusb/libusb/os/darwin_usb.c:390:26: warning: 'kIOMasterPortDefault' is deprecated: first deprecated in macOS 12.0 [-Wdeprecated-declarations]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/IOKit.framework/Headers/IOKitLib.h:123:19: note: 'kIOMasterPortDefault' has been explicitly marked deprecated here
In file included from ../../../../pkg/mod/github.com/karalabe/[email protected]/libs.go:50:
../../../../pkg/mod/github.com/karalabe/[email protected]/libusb/libusb/os/darwin_usb.c:441:60: warning: 'kIOMasterPortDefault' is deprecated: first deprecated in macOS 12.0 [-Wdeprecated-declarations]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/IOKit.framework/Headers/IOKitLib.h:123:19: note: 'kIOMasterPortDefault' has been explicitly marked deprecated here
In file included from ../../../../pkg/mod/github.com/karalabe/[email protected]/libs.go:51:
../../../../pkg/mod/github.com/karalabe/[email protected]/hidapi/mac/hid.c:693:34: warning: 'kIOMasterPortDefault' is deprecated: first deprecated in macOS 12.0 [-Wdeprecated-declarations]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/IOKit.framework/Headers/IOKitLib.h:123:19: note: 'kIOMasterPortDefault' has been explicitly marked deprecated here
# github.com/livepeer/lpms/ffmpeg
decoder.c:234:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]

After a while, it eventually errors:

2022/04/29 17:43:49 Starting container id: a9fad7a881cf image: testcontainers/ryuk:0.3.3
2022/04/29 17:43:49 Waiting for container id a9fad7a881cf image: testcontainers/ryuk:0.3.3
2022/04/29 17:43:49 Container is ready id: a9fad7a881cf image: testcontainers/ryuk:0.3.3
2022/04/29 17:43:49 Starting container id: 3787d385e184 image: darkdragon/geth-with-livepeer-protocol:streamflow
2022/04/29 17:43:49 Container is ready id: 3787d385e184 image: darkdragon/geth-with-livepeer-protocol:streamflow
E0429 17:44:12.132515   79915 handlers.go:1392] HTTP Response Error 500: transaction failed txHash=0xbd9c0231620f44cc8712ca367c2317a1c44312eced285885bac996819cfe40c0
E0429 17:44:12.351076   79915 handlers.go:1392] HTTP Response Error 400: orchestrator already registered

leszko · 2022-05-09T16:57:23Z

If the devtool_utils are only going to be used in the tests then could we move them over to some more specific files/packages within the E2E code?

It's actually not only used in tests, but also in the devtool cmd.

leszko · 2022-05-09T17:00:07Z

Not specific to this PR, but we should look at moving the M1 image to a livepeer-owned repo

There is a dedicated GH Issue which will address this one: #2316

leszko · 2022-05-09T19:02:06Z

I tried running the tests (M1 MacOS. I updated the image name to the darkdragon one) and it seems to hang after this point:

Right, I had the same. And the reason of that is that I rebased to master and there was a bug in master 😱
Fixed in 01750fa.

And for the master I'll send a fix in a separate PR in a few hours.

leszko

Thanks for the review @thomshutt

I addressed your comments. PTAL

leszko · 2022-05-09T16:55:47Z

test/e2e/e2e.go

+		ContainerRequest: req,
+		Started:          true,
+	})
+	if err != nil {


Fixed in 6064454

thomshutt · 2022-05-10T13:25:17Z

test/e2e/register_orchestrator_test.go

+		"amount":         {fmt.Sprintf("%d", lptStake)},
+	}
+
+	for {


Will this infinitely loop in the case where there's something wrong with the activation endpoint?

Good point! Yes, we actually don't use any timeouts in any tests. The full/complete solution we can consider at some point is to use context everywhere with the proper timeout. Saying that it'd slightly complicate the code. I think that for now it's good enough to add -timeout to go test. Added here: 6acddeb

thomshutt · 2022-05-10T13:31:49Z

cmd/devtool/devtool/devtool_utils.go

+func (d *Devtool) InitializeRound() error {
+	// XXX TODO curl -X "POST" http://localhost:$transcoderCliPort/initializeRound
+	time.Sleep(7 * time.Second)
+	for {


Same here, should we have a timeout?

Same as above ⬆️

thomshutt · 2022-05-10T13:32:58Z

cmd/devtool/devtool/devtool_utils.go

+	tx, err := d.Client.InitializeRound()
+	// ErrRoundInitialized
+	if err != nil {
+		if err.Error() != "ErrRoundInitialized" {


Do we need to handle the case where err != nil but it's not this type of error?

No, if the error is "ErrRoundInitialized", then no problem. We don't need to initialize it, because it's already initialized (potentially by someone else).

Saying that, in devtool_util.go, there are a lot of places which could be improved in terms of the code quality, but I don't want to do it this PR. Maybe we can send some following PR, just with the refactoring.

My bad, I'd read err.Error() != "ErrRoundInitialized" as err.Error() == "ErrRoundInitialized"

thomshutt · 2022-05-10T13:47:49Z

Overall looks great, really nice and neat - I played around with making the test fail and added another (duplicate) test to try and check there weren't any race conditions around setup and teardown etc.

Some general thoughts for the future:

Since we didn't go with Cucumber as a meta language, we should try to be strict about keeping new tests looking neat and having Given / When / Then comments like TestRegisterOrchestrator does
It might be good to pipe the log outputs of the processes we start into files (either 1 per test or 1 per process) to make reviewing what happened on test failure easier
The setup / teardown is still a little cumbersome in that you have to remember to wait for the O to be ready and then defer the teardown. We could experiment with grouping tests in the future and doing subtests using the same O with T.Run / T.Parallel after the setup steps https://pkg.go.dev/testing#T.Parallel

leszko · 2022-05-11T13:33:23Z

Thanks for the review!

Since we didn't go with Cucumber as a meta language, we should try to be strict about keeping new tests looking neat and having Given / When / Then comments like TestRegisterOrchestrator does
We still plan to consider Cucumber, please check: Consider using Cucumber for E2E Tests #2331

I think we should convert it to Cucumber (should be quick), send a PR and discuss if we like it more than pure Golang tests.

It might be good to pipe the log outputs of the processes we start into files (either 1 per test or 1 per process) to make reviewing what happened on test failure easier'

Yes, we can consider it. TBH I'd start without it, because producing log files also have drawbacks. Maybe let's check in practice if that's really an issue.

The setup / teardown is still a little cumbersome in that you have to remember to wait for the O to be ready and then defer the teardown. We could experiment with grouping tests in the future and doing subtests using the same O with T.Run / T.Parallel after the setup steps https://pkg.go.dev/testing#T.Parallel

Yes, good idea. Let's wait until we have more E2E Test scenarios and then we can decide if we should run multiple tests (and if yes, then which tests) using the same started Orchestrator.

leszko

@thomshutt Addressed your comments. PTAL.

AlexKordic

Well Done!

AlexKordic · 2022-05-11T19:23:48Z

test/e2e/e2e.go

+	ready := make(chan struct{})
+	go func() {
+		statusEndpoint := fmt.Sprintf("http://%s/status", *lpCfg.CliAddr)
+		var statusCode int
+		for statusCode != 200 {
+			time.Sleep(200 * time.Millisecond)
+			resp, err := http.Get(statusEndpoint)
+			if err == nil {
+				statusCode = resp.StatusCode
+			}
+		}
+		ready <- struct{}{}
+	}()


Awesome! I was wondering how to get a signal when node is ready.

Yeah, also for the context, this is probably good enough for E2E Tests, but we plan to add better health checks to Livepeer:

Register gRPC health check server when running gRPC server as an O #2076

add ability to trigger transcoder health-check from the CLI port #1920

livepeer#2383)

leszko requested review from thomshutt and AlexKordic April 29, 2022 11:18

thomshutt reviewed Apr 29, 2022

View reviewed changes

leszko commented May 9, 2022

View reviewed changes

test/e2e/e2e.go Outdated

ContainerRequest: req,

Started: true,

})

if err != nil {

Copy link

Contributor Author

leszko May 9, 2022

Choose a reason for hiding this comment

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

Fixed in 6064454

leszko requested a review from thomshutt May 9, 2022 19:02

thomshutt reviewed May 10, 2022

View reviewed changes

leszko commented May 11, 2022

View reviewed changes

leszko requested a review from thomshutt May 11, 2022 13:50

thomshutt approved these changes May 11, 2022

View reviewed changes

AlexKordic approved these changes May 11, 2022

View reviewed changes

leszko force-pushed the 2317-e2e-register-orchestrator branch from 6acddeb to e34e65f Compare May 12, 2022 09:02

Add backbone for E2E Tests + First E2E Scenario: Register Orchestrator

fa4bdbc

leszko force-pushed the 2317-e2e-register-orchestrator branch from e34e65f to fa4bdbc Compare May 12, 2022 09:10

leszko merged commit 88f13cc into livepeer:master May 12, 2022

leszko deleted the 2317-e2e-register-orchestrator branch May 12, 2022 10:13

ad-astra-video pushed a commit to ad-astra-video/go-livepeer that referenced this pull request May 25, 2022

Add backbone for E2E Tests + First E2E Scenario: Register Orchestrator (

cf29a96

livepeer#2383)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add foundation for E2E Tests + First E2E Scenario: Register Orchestrator #2383

Add foundation for E2E Tests + First E2E Scenario: Register Orchestrator #2383

leszko commented Apr 29, 2022

codecov bot commented Apr 29, 2022 •

edited

Loading

thomshutt Apr 29, 2022

leszko May 9, 2022

thomshutt commented Apr 29, 2022

thomshutt commented Apr 29, 2022

thomshutt commented May 4, 2022 •

edited

Loading

leszko commented May 9, 2022

leszko commented May 9, 2022

leszko commented May 9, 2022

leszko left a comment

leszko May 9, 2022

thomshutt May 10, 2022

leszko May 11, 2022

thomshutt May 10, 2022

leszko May 11, 2022

thomshutt May 10, 2022

leszko May 11, 2022

thomshutt May 11, 2022

thomshutt commented May 10, 2022

leszko commented May 11, 2022

leszko left a comment

AlexKordic left a comment

AlexKordic May 11, 2022

leszko May 12, 2022

Add foundation for E2E Tests + First E2E Scenario: Register Orchestrator #2383

Add foundation for E2E Tests + First E2E Scenario: Register Orchestrator #2383

Conversation

leszko commented Apr 29, 2022

codecov bot commented Apr 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomshutt commented Apr 29, 2022

thomshutt commented Apr 29, 2022

thomshutt commented May 4, 2022 • edited Loading

leszko commented May 9, 2022

leszko commented May 9, 2022

leszko commented May 9, 2022

leszko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomshutt commented May 10, 2022

leszko commented May 11, 2022

leszko left a comment

Choose a reason for hiding this comment

AlexKordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 29, 2022 •

edited

Loading

thomshutt commented May 4, 2022 •

edited

Loading