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

Ensuring that each ExpectedMetric.Type is unique in IntegrationMetadata #669

Merged
merged 7 commits into from
Jun 21, 2022

Conversation

avilevy18
Copy link
Contributor

@avilevy18 avilevy18 commented Jun 10, 2022

Add validation to ensure ExpectedMetric.Type is unique in IntegrationMetadata and create appropriate tests.

Rename TestAll in common_test.go to TestMetadataValidation

http://b/235595896

@@ -0,0 +1,117 @@
public_url: "https://cloud.google.com/stackdriver/docs/solutions/agents/ops-agent/third-party/activemq"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need such a large test file for this? And does it need real-world data? Should we pare it down to a few entries with generic names?

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably doesn't need real world data, but the whole file is validated at once so it has to pass all validations present in IntegrationMetadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in: a3b3e13

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is no longer real world data, but do we need a file this large to test the validation? Wouldn't it be enough to have 2 (maybe 3) entries?

That said, it does seem to be consistent with the other tests, which also have overgrown test inputs, so maybe we can defer.

Copy link
Contributor Author

@avilevy18 avilevy18 Jun 10, 2022

Choose a reason for hiding this comment

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

I believe we should defer, I initially wanted to trim the test inputs but thought it would be better for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in: f9abb2d

Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@avilevy18 avilevy18 merged commit 2e5fe21 into master Jun 21, 2022
@igorpeshansky igorpeshansky deleted the avilevy-unique-expected-metric-type branch July 10, 2023 21:39
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.

3 participants