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

Adding Acceptance Tests for compose based tests #2357

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

Conversation

daonb
Copy link
Contributor

@daonb daonb commented Nov 22, 2022

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
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

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

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%) ⬇️
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%) ⬇️
operations.go 92.59% <0.00%> (+1.85%) ⬆️

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

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

@daonb daonb requested a review from at-wat November 23, 2022 11:50
@at-wat
Copy link
Member

at-wat commented Nov 24, 2022

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
Copy link
Contributor Author

daonb commented Nov 24, 2022

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
Copy link
Contributor

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

The offer side of the example now includes a -count and -wait options so
the test will eventually end and run faster. It also ends sending and
receiving EOF for clean "answer" exit.
@daonb
Copy link
Contributor Author

daonb commented Dec 23, 2022

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.

a test passes on my machine and fails as part of the workflow.
this change triples the time the offer is side is waiting for a reply
as it's now part of the acceptance tests
restoring the xample to it's original code and adjusting
the test spec
@stv0g stv0g marked this pull request as draft January 23, 2023 09:44
@stv0g stv0g changed the title WIP: Adding Acceptance Testing Specifications Adding Acceptance Testing Specifications Jan 23, 2023
daonb and others added 5 commits January 28, 2023 11:30
with this change the data-channels pion docker works similar to the
other tests and mapping the source directly to /go/src
improve names in the ats workflow
by making sure the DOM is there before try to access it.
Hoping it will make a flaky test stable.
jeremija and others added 11 commits January 30, 2023 15:31
Using an improvment of pion/datachannel, the channel opener can now
set an event to be called when the DATA_CHANNEL_ACK message is recieved

Resolves pion#1063
Relates to pion/datachannel#81
added a wait loop that checks the ensures the sendChanel is open
before hitting send
the code was to protected against a situation where ice gathering
finished before the DOM is loaded. it never happens.
the files will be available in the action's page, and could be
open using playwright trace app.
adding a script to help ensure only the answer will be
printed
problem is, it's all get squeezed in the answer field..
that way I have a chance of figuring out what the error is
when buildign the data channel example
otherwise we get a permission error
@daonb
Copy link
Contributor Author

daonb commented Jan 30, 2023

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
Copy link
Contributor

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

@daonb
Copy link
Contributor Author

daonb commented Jan 31, 2023

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
Copy link
Contributor

ashellunts commented Jan 31, 2023

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
Copy link
Contributor Author

daonb commented Jan 31, 2023

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
Copy link
Contributor

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 daonb changed the title Adding Acceptance Testing Specifications Adding Acceptance Tests for compose based tests Feb 5, 2023
@daonb daonb removed the help wanted label Feb 5, 2023
@daonb daonb marked this pull request as ready for review February 5, 2023 15:13
@daonb
Copy link
Contributor Author

daonb commented Feb 5, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants