-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Generalize testbed #1062
Generalize testbed #1062
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1062 +/- ##
==========================================
+ Coverage 86.73% 86.85% +0.12%
==========================================
Files 201 201
Lines 14513 14518 +5
==========================================
+ Hits 12588 12610 +22
+ Misses 1471 1458 -13
+ Partials 454 450 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for undertaking this.
tc.ValidateData() | ||
} | ||
|
||
func createConfigFile(t *testing.T, sender testbed.DataSender, receiver testbed.DataReceiver, resultDir string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a copy of createConfigFile from scenarios.go or is different from it? If it is a copy perhaps extract as testbed helper func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make things easier to create a configmodels.Config
directly instead of building the yaml string then converting that into a configmodels.Config
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different configuration than used by perf tests. Generating string and converting to Config because functionality does not currently exist to write out Config back to a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an excellent and very useful PR. I left some comments to refine it, let's get it merged asap. I believe it is going to help us catch a lot of correctness problems (as you posted it found a bunch, now we need to analyse them).
Can we run the correctness test as part of the build, but ignore the failures for now?
Commented out the test assertion for zero span diffs so tests are not failed because of diffs but the report is still created. Added correctness tests into the Makefile testbed-runtests and testbed-listtests targets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the replies. Only a couple open comments remaining, please address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only minor output formatting issue left.
@kbrockhoff Thanks a lot! This is a much wanted contribution. |
…telemetry#1062) Extracted out TestResultsSummary (in testbed/testbed/results.go), DataProvider (in testbed/testbed/data_provider.go), OtelcolRunner (in testbed/testbed/otelcol_runner.go), TestCaseValidator (in testbed/testbed/validator.go) interfaces with multiple implementations. Added tracing correctness tests in testbed/correctness using the testbed with different implementations of the 5 interfaces listed than what the perf tests use. **Link to tracking Issue:** Provides the support to cleanly implement open-telemetry#652, open-telemetry#1022, open-telemetry#1023, open-telemetry#1027, open-telemetry#1031 **Testing:** All existing testbed-driven tests still pass. Correctness tests run without any panics. Correctness tests are reporting a number of bugs with translations. **Documentation:** Godocs on all public methods.
* Integration tests for OTel Collector Attributes. * Update CHANGELOG.md * Fix review * Fix review. * Fix test. * Fix test. Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
* Bump operator to 0.95.0 * Bump chainsaw to 0.1.7 * Update otel operator CluterRole Add CRUD permission for PersistentVolume and PersistentVolumeClaim. --------- Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Description: Extracted out TestResultsSummary (in testbed/testbed/results.go), DataProvider (in testbed/testbed/data_provider.go), OtelcolRunner (in testbed/testbed/otelcol_runner.go), TestCaseValidator (in testbed/testbed/validator.go) interfaces with multiple implementations. Added tracing correctness tests in testbed/correctness using the testbed with different implementations of the 5 interfaces listed than what the perf tests use.
Link to tracking Issue: Provides the support to cleanly implement #652, #1022, #1023, #1027, #1031
Testing: All existing testbed-driven tests still pass. Correctness tests run without any panics. Correctness tests are reporting a number of bugs with translations.
Documentation: Godocs on all public methods.