Skip to content

[Metricbeat] Added mage target to run mocket tests#11715

Merged
sayden merged 3 commits intoelastic:masterfrom
sayden:feature/mb/add-mocked-tests-to-mage-targets
Apr 11, 2019
Merged

[Metricbeat] Added mage target to run mocket tests#11715
sayden merged 3 commits intoelastic:masterfrom
sayden:feature/mb/add-mocked-tests-to-mage-targets

Conversation

@sayden
Copy link
Copy Markdown
Contributor

@sayden sayden commented Apr 9, 2019

Split PR from here #11703

Added a mage target called mockedTests to avoid navigating to beats/metricbeat/mb/testing/data to run tests.

@sayden sayden added review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Apr 9, 2019
@sayden sayden self-assigned this Apr 9, 2019
@sayden sayden requested review from andrewkroh and ruflin April 9, 2019 09:01
@sayden sayden requested a review from a team as a code owner April 9, 2019 09:01
@sayden sayden changed the title Atomic commit [Metricbeat] Added mage target to run mocket tests Apr 9, 2019
Copy link
Copy Markdown
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Do you have plans/requirements to use this with x-pack/metricbeat?

Comment thread metricbeat/magefile.go Outdated
goTestDefaultArgs.ExtraFlags = []string{"github.com/elastic/beats/metricbeat/mb/testing/data/."}

if module := os.Getenv("MODULE"); module != "" {
goTestDefaultArgs.ExtraFlags = append(goTestDefaultArgs.ExtraFlags, fmt.Sprintf("-module=%s", module))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For two strings I'd go with: "-module="+module

Comment thread metricbeat/magefile.go Outdated
goTestDefaultArgs.ExtraFlags = append(goTestDefaultArgs.ExtraFlags, fmt.Sprintf("-module=%s", module))
}

if generate := os.Getenv("GENERATE"); generate == "true" || generate == "1" {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For consistency, use the strconv.ParseBool to parse boolean arguments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Comment thread metricbeat/magefile.go Outdated
goTestDefaultArgs.ExtraFlags = append(goTestDefaultArgs.ExtraFlags, "-generate")
}

goTestDefaultArgs.Packages = []string{}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to allocate an empty slice when nil will suffice.

Suggested change
goTestDefaultArgs.Packages = []string{}
goTestDefaultArgs.Packages = nil

Comment thread metricbeat/magefile.go Outdated

goTestDefaultArgs.Packages = []string{}

return mage.GoTest(context.Background(), goTestDefaultArgs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Define the function as func MockedTests(ctx context.Context) error. Then pass the context from Mage through here.

Comment thread metricbeat/magefile.go Outdated
// Use MODULE={module_name} to run only mocked tests with a single module.
// Use GENERATE=true or GENERATE=1 to regenerate JSON files.
func MockedTests() error {
goTestDefaultArgs := mage.DefaultGoTestUnitArgs()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Once you mutate this it will no longer be the default args, so how about just calling it params.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point

@sayden
Copy link
Copy Markdown
Contributor Author

sayden commented Apr 10, 2019

We don't have the requirements to implement this in x-pack yet because none of the modules uses HTTP requests to fetch their data. I think it's better to wait until the first module uses http and then work on the metricbeat/mb/testing/data package to include modules under xpack too

@sayden sayden merged commit 3ba4cc5 into elastic:master Apr 11, 2019
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.

2 participants