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

[Metricbeat] Check that fields are documented in data tests #11127

Merged
merged 5 commits into from
Mar 18, 2019

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Mar 7, 2019

Currently we check in python tests that the fields are documented. As by now we have all the fields also available in the go code with the fields.go files it is possible to do this check in the new data tests.

To prevent cyclic imports the data_test.go had to be moved to its own package.

Now by default all modules are imported. Like this we don't have to add each module manually.

@ruflin ruflin added module review :Testing Team:Integrations Label for the Integrations team labels Mar 7, 2019
@ruflin ruflin requested a review from a team as a code owner March 7, 2019 10:09
@ruflin ruflin mentioned this pull request Mar 7, 2019
10 tasks
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

Tested locally with an undocumented field, and it works!

@ruflin ruflin force-pushed the check-documented branch from f950884 to c627c95 Compare March 11, 2019 08:19
@ruflin ruflin requested a review from a team as a code owner March 11, 2019 08:19
@@ -36,8 +36,8 @@
type: long
description: >
Request latency, number of requests
- name: request.latency.bucket
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not change the template itself as it checks if a * already exists in the name and if not it will add it. We use the * in the docs check to verify that the fields are multifields. We should use this as a convention moving forward.

@ruflin ruflin force-pushed the check-documented branch from c627c95 to 56497fd Compare March 15, 2019 13:19
@@ -190,6 +190,41 @@ func writeDataJSON(t *testing.T, data common.MapStr, module, metricSet string) {
}
}

// checkDocument checks that all fields which show up in the events are documented
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should say checkDocumented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

better then hound ci :-) Changed.

@@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.

package testing
package data
Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly for my own curiosity: why was this package change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remember correctly, without it there was a circular import when using _ "github.com/elastic/beats/metricbeat/include"

ruflin added 5 commits March 18, 2019 09:09
Currently we check in python tests that the fields are documented. As by now we have all the fields also available in the go code with the fields.go files it is possible to do this check in the new data tests.

To prevent cyclic imports the data_test.go had to be moved to its own package.

Now by default all modules are imported. Like this we don't have to add each module manually.
@ruflin ruflin force-pushed the check-documented branch from eeb4ce7 to 4d80a2c Compare March 18, 2019 08:10
@ruflin
Copy link
Contributor Author

ruflin commented Mar 18, 2019

@ycombinator Updated and rebased because of a conflict.

@ruflin ruflin self-assigned this Mar 18, 2019
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@ruflin ruflin merged commit 4dce54b into elastic:master Mar 18, 2019
@ruflin ruflin deleted the check-documented branch March 18, 2019 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants