e2e_test: integrate MistLoadTest to verify streaming #54

emranemran · 2022-07-21T19:20:51Z

Use MistLoadTest to spawn multiple clients for a specific protocol (e.g. HLS) as viewers at catalyst-two node. The test passes if every client is able to connect and stream successfully. The json files generated by MistLoadTest are parse to verify if streams passed or failed.

iameli · 2022-07-21T21:31:16Z

test/e2e/e2e_test.go

+
+		matches, err := filepath.Glob("*" + strings.ToUpper(prot) + "*json")
+		if err != nil {
+			panic(fmt.Errorf("Glob failed: %w", err))


Will defer to @leszko here on best practices for tests like this, but should this maybe be returning an error instead of panicking? Seems like a good practice in general... on the other hand I can see how this error would be like "test harness failure" instead of "failing test" and maybe a panic makes sense there.

Yeah, I'm inclined to remove the panic and just make it a general "fail". We'd be reviewing logs anyways to see where the failures are - doesn't seem like a panic would be anymore useful here.

Yes, better to not use panic in tests. You can do t.Fail() instead.

iameli · 2022-07-21T21:33:44Z

test/e2e/e2e_test.go

+
+		content, err := ioutil.ReadFile(matches[0])
+		if err != nil {
+			glog.Fatalf("Error while opening output file: %s", err)


In any event, no sense in doing anything else after a Fatalf, which will crash the process immediately

Ah I realize that now after looking at the Fatalf man page which calls os.Exit right after. Will fix.

iameli · 2022-07-21T21:35:34Z

test/e2e/e2e_test.go

 		return true
 	}
 	require.Eventually(t, correctStream, 5*time.Minute, time.Second)
+
+	// Define a set of protocols to test at output of node catalyst-two
+	protocols := map[string]string{


Nice! I'm thinking that each protocol should probably be its own test, rather than just having one big loop in here? Minimally, having a separate function for running a protocol test will let you return err in the error cases for those instead of having to do awkward continues or breaks

Right, I think I got a little lost in trying to do things in the "idiomatic" go style. I'll take a look at refactoring.

Yeah, it should be a separate test. Also, the very idiomatic in Go is to use a tests [] array. Please check "Introducing table driven tests" in https://dave.cheney.net/2019/05/07/prefer-table-driven-tests

iameli

Really really nice! Happy it wasn't too terrible to get this working properly - just suggested a few structuring changes

leszko

Added some comments. But two general thoughts:

We should create more tests rather than expanding the assertion of the same test. Having more tests give us better information what failed. One single test is hard to analyze in case of failures.
I'd try to not cleanup files manually, but rather depend on the built-in 1t.TempDir()

leszko · 2022-07-22T08:19:47Z

test/e2e/e2e_test.go

 		return true
 	}
 	require.Eventually(t, correctStream, 5*time.Minute, time.Second)
+
+	// Define a set of protocols to test at output of node catalyst-two
+	protocols := map[string]string{


Yeah, it should be a separate test. Also, the very idiomatic in Go is to use a tests [] array. Please check "Introducing table driven tests" in https://dave.cheney.net/2019/05/07/prefer-table-driven-tests

leszko · 2022-07-22T08:21:25Z

test/e2e/e2e_test.go

+	}
+
+	// rm any old output files from previous MistLoadTest runs
+	cleanupFiles("*json")


Would it be possible to store all these files in the t.TempDir(), then the files are automatically removed after the test is completed.

Manually removing the files has some disadvantages, e.g., if the test fails, then we can end up with uncleaned files.

Yeah, I'll have to update MistLoadUtil to accept a different path. But agree managing files is not the best way - didn't know about t.TempDir() so i'll try to get that to work.

PR here to allow output directory:
DDVTECH/mistserver@1341ffb

This should let me refactor into something much simpler.

leszko · 2022-07-22T08:22:57Z

test/e2e/e2e_test.go

+
+		glog.Infof("Testing %s using url: %s", prot, url)
+
+		cmdMistLoadTest := exec.Command("../../bin/./MistLoadTest", url)


As already commented above, I think this should be a completely separate test, not to add too many things in one test. In general, it's better to have more smaller tests, than one very big test.

leszko · 2022-07-22T08:24:00Z

test/e2e/e2e_test.go

+		}
+		glog.Infof("MistLoadTest stdout: %s", out)
+
+		defer cmdMistLoadTest.Process.Kill()


Is it possible that this process is not killed and we end up with a running MistLoadTest process? E.g. what happens if the line 250 fails?

MistLoadTest will timeout and exit within 30s (default). I'll turn it into a parameter in the next push and verify it does work as expected.

leszko · 2022-07-22T08:24:24Z

test/e2e/e2e_test.go

+	}
+
+	// Parse output results for each protocol tested above
+	var p bool = true


Suggested change

var p bool = true

p := true

leszko · 2022-07-22T08:25:12Z

test/e2e/e2e_test.go

+
+		matches, err := filepath.Glob("*" + strings.ToUpper(prot) + "*json")
+		if err != nil {
+			panic(fmt.Errorf("Glob failed: %w", err))


Yes, better to not use panic in tests. You can do t.Fail() instead.

leszko · 2022-07-22T08:26:17Z

test/e2e/e2e_test.go

+		}
+		fmt.Println(matches)
+
+		content, err := ioutil.ReadFile(matches[0])


What if there are multiple files that are matched? Like there is already another *.json file in the directory?

leszko · 2022-07-22T08:26:44Z

test/e2e/e2e_test.go

+		if err != nil {
+			panic(fmt.Errorf("Glob failed: %w", err))
+		}
+		fmt.Println(matches)


Change to glog

emranemran · 2022-07-29T04:51:02Z

Updates:

Refactored to use table driven test as suggested by @leszko. We can now run a load test against each protocol (e.g. HLS) independently or all protocols at the same time as defined in the test matrix
Using t.TempDir() to get simplify code that tracked/deleted results files
Blocked on merging until @leszko's PR for redirection merges as I need to rebase on that branch to pick up some changes to e2e_test.go

leszko

Added some minor comments. Other than that, it looks good. Thanks for all the changes @emranemran 👍

test/e2e/e2e_test.go

Use MistLoadTest to spawn multiple clients for a specific protocol (e.g. HLS) as viewers at catalyst-two node. The test passes if every client is able to connect and stream successfully. The json files generated by MistLoadTest are parsed to verify if streams passed or failed.

…I host The Mist tester binaries need to run outside of the containers or inside another container. This is a temporary workaround to run outside the containers directly on the CI host. This is obviously a bit fragile and a follow up change will be used to make this more robust by using a third container that contains all relevant test binaries

emranemran requested review from iameli and leszko July 21, 2022 19:20

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch from 172c504 to 73bb142 Compare July 21, 2022 19:27

emranemran requested a review from hjpotter92 July 21, 2022 19:28

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch from 73bb142 to 67d8ad2 Compare July 21, 2022 19:32

iameli reviewed Jul 21, 2022

View reviewed changes

iameli requested changes Jul 21, 2022

View reviewed changes

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch from 67d8ad2 to c3ca3ca Compare July 22, 2022 06:17

leszko requested changes Jul 22, 2022

View reviewed changes

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch 2 times, most recently from 0ad11df to dc7fd92 Compare July 29, 2022 04:27

emranemran changed the base branch from main to rafal/playback-redirection July 29, 2022 04:28

emranemran changed the base branch from rafal/playback-redirection to main July 29, 2022 04:33

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch 2 times, most recently from 0ad11df to 5cc69cd Compare July 29, 2022 04:46

emranemran requested review from leszko and iameli July 29, 2022 04:47

leszko approved these changes Jul 29, 2022

View reviewed changes

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

test/e2e/e2e_test.go Show resolved Hide resolved

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch 8 times, most recently from 8acd8d7 to f3e6cda Compare August 1, 2022 19:06

.drone.yaml: docker build --no-cache

29324a6

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch from 0a38628 to 3169383 Compare August 2, 2022 22:02

emranemran force-pushed the user/emahbub/integrate-mistloadtest-e2e branch from 3169383 to 1f91195 Compare August 2, 2022 22:06

emranemran mentioned this pull request Aug 2, 2022

e2e test: create a new container to hold all testing utilities like ffmpeg/MistLoadTest/MistAnalyser* #83

Open

emranemran merged commit 277bf98 into main Aug 2, 2022

iameli mentioned this pull request Aug 3, 2022

WIP: integrate mistloadtest e2e 2 #62

Closed

emranemran deleted the user/emahbub/integrate-mistloadtest-e2e branch August 3, 2022 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e_test: integrate MistLoadTest to verify streaming #54

e2e_test: integrate MistLoadTest to verify streaming #54

emranemran commented Jul 21, 2022

iameli Jul 21, 2022

emranemran Jul 22, 2022

leszko Jul 22, 2022

iameli Jul 21, 2022

emranemran Jul 22, 2022

iameli Jul 21, 2022

emranemran Jul 22, 2022

leszko Jul 22, 2022

iameli left a comment

leszko left a comment

leszko Jul 22, 2022

leszko Jul 22, 2022

emranemran Jul 22, 2022

emranemran Jul 26, 2022

leszko Jul 22, 2022

leszko Jul 22, 2022

emranemran Jul 22, 2022

leszko Jul 22, 2022

leszko Jul 22, 2022

leszko Jul 22, 2022

leszko Jul 22, 2022

emranemran commented Jul 29, 2022

leszko left a comment


		glog.Infof("Testing %s using url: %s", prot, url)

		cmdMistLoadTest := exec.Command("../../bin/./MistLoadTest", url)

e2e_test: integrate MistLoadTest to verify streaming #54

e2e_test: integrate MistLoadTest to verify streaming #54

Conversation

emranemran commented Jul 21, 2022

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

iameli left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emranemran commented Jul 29, 2022

leszko left a comment

Choose a reason for hiding this comment