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

Add moduleFlag, omitDocumentedFieldsCheck and ModuleConfig to http testing framework #11660

Merged
merged 4 commits into from
Apr 8, 2019

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Apr 4, 2019

  • moduleFlag: While working on the first days with the new testing framework I found a common pattern which was to re-launch tests to check a specific module. In its current state you must launch all tests and you cannot select a single one. Using go test . -module=apache you can test only that module
  • module_config some modules have specific fields that are required in order them to work. http/json metricset is an example of a required field (namespace). module_config map-field in config.yml adds the map to the config params of a module. For example with this config.yml in http/json/_meta/testdata module:
type: http
url: "/hello"
suffix: json
module:
  namespace: test
  foo: bar

The following module config is produced:

- module: http
  metricsets:
    - json
  period: 10s
  hosts: ["localhost:80"]
  path: "/"
  namespace: "test"
  foo: bar
  • omitDocumentedFieldsCheck: Is a new config field to omit certain fields from being checked inside the documentation. As described in the code: OmitDocumentedFieldsCheck is a list of fields that must be omitted from the function that checks if the field is contained in {metricset}/_meta/fields.yml
type: http
url: "/hello"
suffix: json
omitDocumentedFieldsCheck:
  - hello
  - world.*
  - *

@sayden sayden added review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Apr 4, 2019
@sayden sayden self-assigned this Apr 4, 2019
@sayden sayden requested a review from ruflin April 4, 2019 21:43
@sayden sayden requested a review from a team as a code owner April 4, 2019 21:43
Type string
URL string
Suffix string
ModuleConfig map[string]interface{} `yaml:"module_config"`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should call it just config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was that a reader can clearly understand that anything in this field is module related configuration and not test related configuration.

What about calling it just module then? If the reason is to short it, the file is already called config.yml so the fact that some configuration is going to be find inside is assumed.

  • config only could lead to confusion as soon as we need any other config, for example metricset config (so we don't end up with config, which refers to module config and metricsetConfig for something specific of the metricset).
  • An alternative is to call it config.Module so later we can also have config.Metricset but I think that as the file is called config.yml, prefixing a field with config. is redundant as well as placing it in the suffix like now.

At the same time, if we maintain the variables names like config in data_test.go code, calls to this module configuration will look like config.Module instead of config.Config.

metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
@kaiyan-sheng
Copy link
Contributor

It's great to have the ability to run go test for a specific module. I'm trying to test it locally, do I need to run go test . -module=apache under a specific directory? I tried to run it under metricbeat and it says flag provided but not defined: -module 😄

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I like calling the config option module.

metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Outdated Show resolved Hide resolved
metricbeat/mb/testing/data/data_test.go Show resolved Hide resolved
@sayden sayden changed the title Add moduleFlag and ModuleConfig to http testing framework Add moduleFlag, omitDocumentedFieldsCheck and ModuleConfig to http testing framework Apr 8, 2019
@sayden
Copy link
Contributor Author

sayden commented Apr 8, 2019

It's great to have the ability to run go test for a specific module. I'm trying to test it locally, do I need to run go test . -module=apache under a specific directory? I tried to run it under metricbeat and it says flag provided but not defined: -module

Yes! You must run it from metricbeat/mb/testing/data

@sayden
Copy link
Contributor Author

sayden commented Apr 8, 2019

Error seems unrelated. Merging

@sayden sayden merged commit e8342d8 into elastic:master Apr 8, 2019
@kaiyan-sheng
Copy link
Contributor

@sayden Thanks 👍 I will try it again!!

@sayden sayden deleted the improve-http-testing-framework branch August 25, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants