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.

Sign up for GitHub

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

Jump to bottom

processor/deltatocumulative: golden tests #35562

Merged
merged 1 commit into from
Oct 18, 2024
Merged

processor/deltatocumulative: golden tests #35562

merged 1 commit into from
Oct 18, 2024

Conversation

sh0rez
Copy link
Member

@sh0rez sh0rez commented Oct 2, 2024

Description:

Rewrites most tests to use a pkg/golden like approach:

  • tests are directories under testdata/
  • tests consist of multiple stages, ran in series
  • each stage is a txtar, containing:
    • in: pmetric yaml of the ConsumeMetrics input
    • out: expected output on the sink after calling ConsumeMetrics

The multi-stage setup allows to exercise multiple Requests in one test.

Using txtar allows to co-locate common yaml easily. I plan to add metric assertions in there later too.

Link to tracking Issue: none

Testing: Tests were rewritten

Documentation: not needed

@sh0rez sh0rez requested review from jpkrohling and a team as code owners October 2, 2024 19:46
@github-actions github-actions bot requested a review from RichieSams October 2, 2024 19:47
@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Oct 3, 2024
@sh0rez sh0rez force-pushed the deltatocumulative-testing branch from 7cc6fc2 to 9c28d4f Compare October 3, 2024 06:59
ArthurSens
ArthurSens approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to understand this testar package, but after a few minutes debugging it looks pretty cool :)

I really like the usage of testdata, it covers everything and it's a lot easier to understand than the previous test!

LGTM, just one question

processor/deltatocumulativeprocessor/testdata/tracking/generate.go Outdated Show resolved Hide resolved
@RichieSams
Copy link
Contributor

Overall, I think it's great. Just had a small comment about adding some text documentation to the code for future readers

@sh0rez sh0rez requested a review from RichieSams October 4, 2024 13:43
@sh0rez sh0rez force-pushed the deltatocumulative-testing branch from 140f4d1 to 60461b8 Compare October 4, 2024 13:48
@sh0rez
Copy link
Member Author

sh0rez commented Oct 8, 2024

@RichieSams do you approve?

@sh0rez sh0rez force-pushed the deltatocumulative-testing branch 2 times, most recently from 7a49aea to 9be9f26 Compare October 11, 2024 12:44
@sh0rez sh0rez mentioned this pull request Oct 11, 2024
Merged
@sh0rez sh0rez force-pushed the deltatocumulative-testing branch from 9be9f26 to ac7080f Compare October 11, 2024 12:54
@sh0rez sh0rez mentioned this pull request Oct 15, 2024
Merged
@sh0rez
processor/deltatocumulative: golden tests
acc6f13 
Rewrites most tests to use a pkg/golden like approach
@sh0rez sh0rez force-pushed the deltatocumulative-testing branch from ac7080f to acc6f13 Compare October 18, 2024 15:03
@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 18, 2024
@mx-psi mx-psi merged commit 633ed51 into open-telemetry:main Oct 18, 2024
167 of 168 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 18, 2024
@sh0rez sh0rez mentioned this pull request Oct 18, 2024
Closed
@ArthurSens ArthurSens mentioned this pull request Oct 18, 2024
Closed
songy23
songy23 reviewed Oct 18, 2024
View reviewed changes
processor/deltatocumulativeprocessor/processor_test.go
err := proc.ConsumeMetrics(context.Background(), rms)
require.NoError(t, err)
stages, _ := filepath.Glob(file("*.test"))
for _, file := range stages {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails in Windows, see /~https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11406634691/job/31740841412

Copy link
Member

@ArthurSens ArthurSens Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I just now realized that window tests were skipped in this PR. Pull requests targeting main shouldn't skip this, right?

It will be useful if windows tests run in a follow-up PR fixing the problem here :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to get access to a windows machine to understand the failure. cc @sh0rez in case you beat me to it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are skipped unless the 'Run Windows' label was applied:

opentelemetry-collector-contrib/.github/workflows/build-and-test-windows.yml

Line 49 in 1ef15b9

if: ${{ github.actor != 'dependabot[bot]' && (contains(github.event.pull_request.labels.*.name, 'Run Windows') || github.event_name == 'push' || github.event_name == 'merge_group') }}

If you need to, feel free to file a draft PR, ping me and I can add the 'Run Windows' label so you can use the PR for testing

@songy23 songy23 mentioned this pull request Oct 21, 2024
Closed
@RichieSams
Copy link
Contributor

Nice job with this

@ArthurSens ArthurSens mentioned this pull request Oct 21, 2024
Merged
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
@sh0rez @sbylica-splunk
processor/deltatocumulative: golden tests (open-telemetry#35562)
4c9cf7b 
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

Rewrites most tests to use a pkg/golden like approach:

- tests are directories under `testdata/`
- tests consist of multiple stages, ran in series
- each stage is a [txtar](https://pkg.go.dev/golang.org/x/tools/txtar),
containing:
  - `in`: pmetric yaml of the `ConsumeMetrics` input
  - `out`: expected output on the sink after calling `ConsumeMetrics`

The multi-stage setup allows to exercise multiple Requests in one test.

Using txtar allows to co-locate common yaml easily. I plan to add metric
assertions in there later too.

**Link to tracking Issue:** none

**Testing:** Tests were rewritten

**Documentation:** not needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArthurSens ArthurSens ArthurSens approved these changes

@songy23 songy23 songy23 left review comments

@mx-psi mx-psi mx-psi approved these changes

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

@RichieSams RichieSams Awaiting requested review from RichieSams

Assignees

@dmitryax dmitryax

Labels
cmd/otelcontribcol otelcontribcol command processor/deltatocumulative Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
v0.112.0
Development

Successfully merging this pull request may close these issues.
6 participants
@sh0rez @RichieSams @mx-psi @songy23 @ArthurSens @dmitryax