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

improve e2e test #111

Merged
merged 2 commits into from
Aug 6, 2023
Merged

improve e2e test #111

merged 2 commits into from
Aug 6, 2023

Conversation

ortex
Copy link
Member

@ortex ortex commented Jul 2, 2023

fixes #101

@coveralls
Copy link
Collaborator

coveralls commented Jul 2, 2023

Coverage Status

coverage: 95.037%. remained the same when pulling 5529624 on improve_e2e_test into c859e77 on main.

@gavv
Copy link
Member

gavv commented Jul 28, 2023

Nice!

I few suggestions:

  • Test waits until it receives first non-zero sample, then receives 10000 samples, then checks that each one is either zero or has correct value; it means that if we received one non-zero sample, and then 10000 zeros, the test will pass. Instead of this, I suggest to remove upper limit for how much samples we receive, and instead place lower limit of how much non-zero samples we want. So we will receive samples, check that each one is either zero or correct, and repeat this until we accumulate N non-zero samples. This way the test will check that there are a lot (N) of good samples, and at the same time it won't be bound to a particular loss ratio. Losses may happen on a loaded CI worker.

  • 100 samples is not very interesting number - they will fit in one packet. I suggest to wait until we receive something like 10K non-zero samples, so that it will be many packets.

  • To avoid unintuitive part with 0.5, we can work with integers:

    valueL = int(samples[i*2] * 100)
    assert((prevValue + 1) % 100 == valueL)
    
  • When assertions fails, it makes sense to report index and maybe dump the whole stream or block.

  • Let's replace magical number 2 with a constant like NumChan(nel)s.

@gavv gavv added the needs revision Pull request should be revised by its author label Jul 28, 2023
@ortex
Copy link
Member Author

ortex commented Aug 6, 2023

  • When assertions fails, it makes sense to report index and maybe dump the whole stream or block.
--- FAIL: TestEnd2End_Default (0.42s)
    e2e_test.go:146: 
                Error Trace:    /Users/andreybushmin/workspace/roc/roc-go/roc/e2e_test.go:146
                Error:          Not equal: 
                                expected: 2
                                actual  : 3
                Test:           TestEnd2End_Default
                Messages:       prevL: 1, valueL: 2, index: 2, samples: [0=0.01, 1=-0.01, 2=0.02, 3=-0.02, 4=0.03, 5=-0.03, 6=0.04, 7=-0.04, 8=0.05, 9=-0.05, 10=0.06, 11=-0.06, 12=0.07, 13=-0.07, 14=0.08, 15=-0.08, 16=0.09, 17=-0.09, 18=0.10, 19=-0.10, 20=0.11, 21=-0.11, 22=0.12, 23=-0.12, 24=0.13, 25=-0.13, 26=0.14, 27=-0.14, 28=0.15, 29=-0.15, 30=0.16, 31=-0.16, 32=0.17, 33=-0.17, 34=0.18, 35=-0.18, 36=0.19, 37=-0.19, 38=0.20, 39=-0.20, 40=0.21, 41=-0.21, 42=0.22, 43=-0.22, 44=0.23, 45=-0.23, 46=0.24, 47=-0.24, 48=0.25, 49=-0.25, 50=0.26, 51=-0.26, 52=0.27, 53=-0.27, 54=0.28, 55=-0.28, 56=0.29, 57=-0.29, 58=0.30, 59=-0.30, 60=0.31, 61=-0.31, 62=0.32, 63=-0.32, 64=0.33, 65=-0.33, 66=0.34, 67=-0.34, 68=0.35, 69=-0.35, 70=0.36, 71=-0.36, 72=0.37, 73=-0.37, 74=0.38, 75=-0.38, 76=0.39, 77=-0.39, 78=0.40, 79=-0.40, 80=0.41, 81=-0.41, 82=0.42, 83=-0.42, 84=0.43, 85=-0.43, 86=0.44, 87=-0.44, 88=0.45, 89=-0.45, 90=0.46, 91=-0.46, 92=0.47, 93=-0.47, 94=0.48, 95=-0.48, 96=0.49, 97=-0.49, 98=0.50, 99=-0.50]

output example (changed assert to fail)

all other suggestions are fixed

@gavv gavv merged commit 895c183 into main Aug 6, 2023
8 checks passed
@gavv gavv deleted the improve_e2e_test branch August 6, 2023 11:45
@gavv
Copy link
Member

gavv commented Aug 6, 2023

Thanks!!

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.

Improve acceptance criteria of end-to-end test
3 participants