Adding Acceptance Tests for compose based tests

[daonb]

Description

There's a black box testing pattern I've been using in my projects and I'm happy to contribute. It's based on docker compose and playwright. For now, there are three tests:

• pion-to-pion
• e2e
• the data channel example which runs over the Firefox and Chromium browsers

I've made as little changes as possible to the example: adding data-test-id in a couple of places and making main.go respond promptly on establishing a connection. It could be premature optimization, but waiting 5 seconds every time seems silly.

[codecov[bot]]

Codecov Report

Base: 77.55% // Head: 77.51% // Decreases project coverage by -0.04% :warning:

Coverage data is based on head (38fd0b4) compared to base (31c8f0a). Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2357      +/-   ##
==========================================
- Coverage   77.55%   77.51%   -0.04%     
==========================================
  Files          87       87              
  Lines        9292     9292              
==========================================
- Hits         7206     7203       -3     
- Misses       1652     1654       +2     
- Partials      434      435       +1     

Flag	Coverage Δ
go	79.28% <ø> (-0.04%)	:arrow_down:
wasm	70.15% <ø> (ø)

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

Impacted Files	Coverage Δ
internal/mux/mux.go	84.78% <0.00%> (-4.35%)	:arrow_down:
operations.go	92.59% <0.00%> (+1.85%)	:arrow_up:

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

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[at-wat]

FYI, I added E2E test before to confirm audio through WebRTC is received by browser. https://github.com/pion/webrtc/tree/master/e2e and it's running on CI https://github.com/pion/webrtc/blob/master/.github/workflows/browser-e2e.yaml It would be nice to combine the acceptance test and existing e2e test, and run on CI in future.

I feel that the test application is better to be separated from examples like the above e2e dir.

[daonb]

FYI, I added E2E test before to confirm audio through WebRTC is received by browser.

https://github.com/pion/webrtc/tree/master/e2e

and it's running on CI

https://github.com/pion/webrtc/blob/master/.github/workflows/browser-e2e.yaml

It would be nice to combine the acceptance test and existing e2e test, and run on CI in future.

Thanks for that test, Sean quoted it on slack and I tracked you down for this review ;-)

Once this PR is ready and merged, I'll be happy to work together on "folding" your tests to the new infrastructure and get it to run both in the CI and on devs hosts.

I feel that the test application is better to be separated from examples like the above e2e dir.

I'd rather not create separate directories for e2e, examples, etc. and keep it flat for two reasons:

• ./ats is already deep and sparse compared to root which has 166 files.
• as we have more tests and more types of tests it'll become harder to find tests as the distinction between e2e to integration to system is not always that clear.

What we can do is have a naming convention for the tests directories: Have all e2e tests start with "e2e_" and examples start with "ex_", integration with "int_", system tests with "sys_" and so on.

This way ll ats output will be grouped by test type instead of a mishmash of tests of different types in random order. It'll be easier to read and hurt our eyes less.

[ashellunts]

FYI, there is a e2e test here https://github.com/pion/webrtc/blob/master/examples/pion-to-pion/test.sh

[daonb]

Happy holidays! I've just pushed a couple of commits to bring into ats the pion-to-pion & e2e tests. I still need to refactor the data-channel as I learned a lot in the process.

I copied the e2e test as is and in the process came to realize we need to refactor it. Today it runs on a single container and it's better to have at least two as we can separate the client from the server.

From the pion-to-pion example I did a few changes:

• added a count with max int default so the CI can make it end
• added "EOF" for orderly shutdown
• make the "offer" retry the HTTP post is case "answer" is late to the party
• removed a .sh file and a couple of Dockerfile
• update the README with the command to run as a test

With these changes all I needed to add is a lab.yaml file:

version: '3'
services:
  answer:
    image: golang:1.19
    volumes:
      - .:/go/src/github.com/pion/webrtc
    working_dir: /go/src/github.com/pion/webrtc/examples/pion-to-pion/answer
    command: go run . -offer-address runner:50000

  runner:
    depends_on:
      - answer
    image: golang:1.19
    volumes:
      - .:/go/src/github.com/pion/webrtc
    working_dir: /go/src/github.com/pion/webrtc/examples/pion-to-pion/offer
    command: go run . -answer-address answer:60000 -count 3 -wait 1

It's a template I still need to copy back to the data-channels. To use it for your test, all we need to change is the services names, working dirs and command args.

I also haven't finished work on the CI - replacing the e2e with running ./ats/run to run all the tests.

[daonb]

All three tests are passing and are running in the CI. All that's left is getting the folder name right. We have ats now but I don't like it. I'd like to change it to atps as Acceptance Test Procedures are a well known engineering practice. Please reply with a better names or add 👍🏽 if I can go ahead and change the name.

[ashellunts]

What do you think about name "end-to-end-tests"? or "e2e-tests"?

[daonb]

I like e2e as a concept but it's less fitting here.

As a library, webrtc doesn't come with an "end". To test e2e we have to either include a test only frontend or one of the examples. In any case it's not truly an end-to-end test.

Another reason I prefer Acceptance Test Procedure is that e2e tests are only one of the kinds of tests this infrastructure support. The infrastructure can support any black-box test we need. We can use it for interoperability tests, integration tests, component tests, functional tests, etc.

[ashellunts]

I like e2e as a concept but it's less fitting here.

As a library, webrtc doesn't come with an "end". To test e2e we have to either include a test only frontend or one of the examples. In any case it's not truly an end-to-end test.

Another reason I prefer Acceptance Test Procedure is that e2e tests are only one of the kinds of tests this infrastructure support. The infrastructure can support any black-box test we need. We can use it for interoperability tests, integration tests, component tests, functional tests, etc.

But we do include tests with front end (browser-e2e) and tests that run an example apps (pion-to-pion).

May be it is only me, but I don't understand what an acceptance test is in software development. If it is to "accept" software as working and ready for release, then all tests are acceptance tests.

I am not against the name, just curios. I would only use more verbose name, like acceptance_tests, not ats. In general really great changes to unify existing and future tests.

[daonb]

Thank you for your kind words and questions. You're right - all our tests are acceptance tests. Some of them are also unit tests (aka white box testing) and for those we follow Go's pattern. For all other acceptance tests we have this virtual lab infrastructure in a special folder. "Acceptance Tests" is the name of the workflow I added, maybe I should move unit testing there?

As for the name, let me give you more context. This is the third time I'm adding this to a project. It started with Terminal7 where I used it to verify my trio of client, server and signaling server are playing well together. It has some e2e tests but most are integration tests for connection and disconnection scenarios.

I then copied it to the server and wrote a test for a new CLI subcommand. I guess in this case it's an e2e test as the test starts with CLI. I believe it doesn't really matter if it's an e2e or an ABC type of test. As long as we have more tests, fewer bugs will slip through.

I was once in a project where the type of tests did matter. We were developing avionics software for the F16 fighter jet and it was a very carefully engineered project. There was even a military standard for the waterfall process we were using. There I first met the ATPs - meticulously planned test flights to ensure the systems work and the bombs hit their targets.

In this day and age, we don't plan like this (I guess some unlucky few still do). Any test is welcome and we should encourage contributors to add any acceptance tests for their code. ATPs are loosely defined and it's a good thing as they can cover any kind of test contributors will come up with.

[ashellunts]

Thank you for your kind words and questions. You're right - all our tests are acceptance tests. Some of them are also unit tests (aka white box testing) and for those we follow Go's pattern. For all other acceptance tests we have this virtual lab infrastructure in a special folder. "Acceptance Tests" is the name of the workflow I added, maybe I should move unit testing there?

As for the name, let me give you more context. This is the third time I'm adding this to a project. It started with Terminal7 where I used it to verify my trio of client, server and signaling server are playing well together. It has some e2e tests but most are integration tests for connection and disconnection scenarios.

I then copied it to the server and wrote a test for a new CLI subcommand. I guess in this case it's an e2e test as the test starts with CLI. I believe it doesn't really matter if it's an e2e or an ABC type of test. As long as we have more tests, fewer bugs will slip through.

I was once in a project where the type of tests did matter. We were developing avionics software for the F16 fighter jet and it was a very carefully engineered project. There was even a military standard for the waterfall process we were using. There I first met the ATPs - meticulously planned test flights to ensure the systems work and the bombs hit their targets.

In this day and age, we don't plan like this (I guess some unlucky few still do). Any test is welcome and we should encourage contributors to add any acceptance tests for their code. ATPs are loosely defined and it's a good thing as they can cover any kind of test contributors will come up with.

Thank you for the thorough answer. Regarding unit tests, in my opinion it is good to have separate GitHub workflow for them. Because it gives transparency on what tests fail/pass (unit or acceptance).

[daonb]

I left the unit test where it was and changed the directory name to acceptance-tests