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

[componenttest] Move CheckConfigStruct to confmaptest #12580

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

evan-bradley
Copy link
Contributor

Description

This function is mostly concerned with config unmarshaling, so it relates much more to confmap than it does to component.

Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 87.01299% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.16%. Comparing base (8cf42f3) to head (75132f3).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
confmap/confmaptest/configtest.go 87.01% 8 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (87.01%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12580      +/-   ##
==========================================
- Coverage   92.18%   92.16%   -0.02%     
==========================================
  Files         469      469              
  Lines       25390    25471      +81     
==========================================
+ Hits        23405    23476      +71     
- Misses       1574     1582       +8     
- Partials      411      413       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@evan-bradley
Copy link
Contributor Author

Contrib tests are broken due to the changes in mdatagen; will need a make update-otel after this is merged. I'm skipping the contrib tests for now.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

One small hiccup with this: we split all the test modules except for confmaptest, so this will be moved from a 0.x module to a 1.x module. That's okay for me but wanted to highlight it

Comment on lines +23 to 24
// Deprecated: [v0.122.0] Use confmaptest.CheckConfigStruct instead.
func CheckConfigStruct(config any) error {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Deprecated: [v0.122.0] Use confmaptest.CheckConfigStruct instead.
func CheckConfigStruct(config any) error {
// Deprecated: [v0.122.0] Use confmaptest.CheckConfigStruct instead.
var CheckConfigStruct = confmaptest.CheckConfigStruct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, I'll do that.

@evan-bradley
Copy link
Contributor Author

One small hiccup with this: we split all the test modules except for confmaptest, so this will be moved from a 0.x module to a 1.x module.

I missed that, thanks for pointing it out. I know I want to make at least one change to this function, I'm going to make this a bit more airtight before I move it.

@evan-bradley evan-bradley marked this pull request as draft March 7, 2025 16:47
@evan-bradley
Copy link
Contributor Author

@mx-psi @bogdandrutu Please see #12590. From what I can tell, we can get away without any breaking changes, but I want to confirm we're okay with how that works before we move forward.

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

Successfully merging this pull request may close these issues.

3 participants