Pass go test flags to integration tests #3109

pchila · 2023-07-20T15:59:38Z

What does this PR do?

Why is it important?

It allows to propagate go test flags to the actual go test command allowing for finer control over the test execution.
The -test.short is also important for allowing a quicker execution for PRs, for example while maintaining a full integration test run in other contexts.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
~~[ ] I have made corresponding change to the default configuration files~~
~~[ ] I have added tests that prove my fix is effective or that my feature works~~
~~[ ] I have added an entry in ./changelog/fragments using the changelog tool~~
~~[ ] I have added an integration test or an E2E test~~

Author's Checklist

[ ]

How to test this PR locally

Try to run integration tests (both locally and remotely) with some custom go test flags. There are some examples listed in the dev-guide, for example:

We pass a -test.run flag along with the names of the tests we want to run in OR

GOTEST_FLAGS="-test.run ^(TestStandaloneUpgrade|TestFleetManagedUpgrade)$" mage integration:test

Related issues

Relates Introduce upgrade tests from released versions #2955

Use cases

Screenshots

Logs

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

mergify · 2023-07-20T16:00:14Z

This pull request does not have a backport label. Could you fix it @pchila? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2023-07-20T16:34:01Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-07-26T08:03:38.194+0000
Duration: 24 min 28 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6127
Skipped	27
Total	6154

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

pchila · 2023-07-24T07:04:48Z

/test

elasticmachine · 2023-07-24T07:33:19Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.718% (`77/78`)	👍
Files	68.657% (`184/268`)	👍
Classes	67.606% (`336/497`)	👍
Methods	54.222% (`1053/1942`)	👍
Lines	40.331% (`12049/29875`)	👍 0.043
Conditionals	100.0% (`0/0`)	💚

elasticmachine · 2023-07-24T09:05:00Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

magefile.go

blakerouse

Change looks good to me.

magefile.go

pchila · 2023-07-25T09:37:52Z

ok, now integration tests are running correctly after a small change in 5c32270

/cc @ycombinator

ycombinator · 2023-07-25T16:32:04Z

dev-tools/mage/gotest.go

@@ -232,11 +232,10 @@ func GoTest(ctx context.Context, params GoTestArgs) error {
 		)
 	}

+	testArgs = append(testArgs, params.ExtraFlags...)


Just curious: why was this change needed?

Sorry for the incoming wall of text 😅

TL;DR: This was needed to make sure that a -test.run flag specified in the GOTEST_FLAGS environment variable does not interfere with the batching done by the framework.

Full explanation:
The integration test framework runs the tests twice:

the first time we pass a special tag that make all the define statements in the tests skip the test and dump the requirements. This output is processed by the integration test framework to discover the tests and the set of environments/machines we will need to spawn and allocate the tests to the various machines.

the second time we run the tests (here) the integration test framework adds a -test.run flag when launching go test on the remote machine to make sure that only the tests corresponding to that batch are executed.

By specifying the extra flags before the -test.run for the batch we make sure that the last flag definition "wins" (have a look at the unit test in batch_test.go) so that whatever run constraint is specified in GOTEST_FLAGS participates in the discovery and batching (1st go test execution) but doesn't override the actual execution on the remote machine (2nd go test execution).

This should definitely be a comment in the code.

Added the comment just above the change.

docs/test-framework-dev-guide.md

ycombinator · 2023-07-25T16:38:13Z

magefile.go

@@ -1491,7 +1506,7 @@ func integRunner(ctx context.Context, matrix bool, singleTest string) error {
 	return nil
 }

-func createTestRunner(matrix bool, singleTest string, batches ...define.Batch) (*runner.Runner, error) {
+func createTestRunner(matrix bool, singleTest string, goTestFlags string, batches ...define.Batch) (*runner.Runner, error) {


Could we get rid of the singleTest argument and the corresponding mage integration:single target now that developers could set GOTEST_FLAGS="-test.run ^SingleTestName$ instead? I think it would be less confusing to have just one way to specify which test(s) to run.

We could... I am leaving it in, in case the new flags do not behave like we want but we can deprecate them and create a separate issue to clean up

I like mage integration:single [testName] it is much clean CLI command to run a test than all the ENV flags being set. I wish we could get mage to take command line arguments for targets. I would prefer to see something like:

mage integration:test -short -name SingleTestName -platform linux/amd64

Okay, then could we eliminate just the singleTest argument to this function and have mage integration:single [testName] internally setup goTestFlags appropriately?

docs/test-framework-dev-guide.md

ycombinator

Left a couple of questions and suggestions.

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>

pchila · 2023-07-25T18:20:42Z

/test

pchila · 2023-07-26T07:52:54Z

/test

…oTest

pchila · 2023-07-26T08:50:53Z

buildkite test this

* Pass go test flags to integration tests * Update docs/test-framework-dev-guide.md * Add clarification about ExtraFlags and RunExpr ordering in devtools.GoTest --------- Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com> (cherry picked from commit e54b0a2)

* Pass go test flags to integration tests * Update docs/test-framework-dev-guide.md * Add clarification about ExtraFlags and RunExpr ordering in devtools.GoTest --------- Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com> (cherry picked from commit e54b0a2) Co-authored-by: Paolo Chilà <paolo.chila@elastic.co>

pchila added enhancement New feature or request skip-changelog Testing labels Jul 20, 2023

pchila requested review from ycombinator, AndersonQ, belimawr and blakerouse July 20, 2023 15:59

pchila requested a review from a team as a code owner July 20, 2023 15:59

pchila self-assigned this Jul 20, 2023

mergify bot added the backport-skip label Jul 20, 2023

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Jul 24, 2023

pierrehilbert added the backport-v8.9.0 Automated backport with mergify label Jul 24, 2023

mergify bot removed the backport-skip label Jul 24, 2023

belimawr reviewed Jul 24, 2023

View reviewed changes

magefile.go Outdated Show resolved Hide resolved

pchila added 3 commits July 24, 2023 18:14

Pass go test flags to integration tests

d416f4b

fixup! Pass go test flags to integration tests

bf1213a

fixup! fixup! Pass go test flags to integration tests

4c0a8b9

pchila force-pushed the pass-test-flags-to-integration-tests branch from 038ff44 to 4c0a8b9 Compare July 24, 2023 16:14

blakerouse approved these changes Jul 25, 2023

View reviewed changes

magefile.go Outdated Show resolved Hide resolved

fixup! fixup! fixup! Pass go test flags to integration tests

5c32270

pchila force-pushed the pass-test-flags-to-integration-tests branch from 1bb7bc8 to 5c32270 Compare July 25, 2023 08:04

pchila added 2 commits July 25, 2023 11:30

fixup! fixup! fixup! fixup! Pass go test flags to integration tests

bdc7055

Implement feedback for review

c54cc9f

pchila requested a review from belimawr July 25, 2023 09:38

ycombinator reviewed Jul 25, 2023

View reviewed changes

docs/test-framework-dev-guide.md Show resolved Hide resolved

ycombinator reviewed Jul 25, 2023

View reviewed changes

docs/test-framework-dev-guide.md Show resolved Hide resolved

ycombinator reviewed Jul 25, 2023

View reviewed changes

Update docs/test-framework-dev-guide.md

8daeac2

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>

Add clarification about ExtraFlags and RunExpr ordering in devtools.G…

9408009

…oTest

pchila merged commit e54b0a2 into elastic:main Jul 26, 2023

mergify bot mentioned this pull request Jul 26, 2023

[8.9](backport #3109) Pass go test flags to integration tests #3130

Merged

pchila linked an issue Jul 27, 2023 that may be closed by this pull request

Integration Testing Framework: add support for arbitrary go test flags #3028

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass go test flags to integration tests #3109

Pass go test flags to integration tests #3109

pchila commented Jul 20, 2023 •

edited

Loading

mergify bot commented Jul 20, 2023

elasticmachine commented Jul 20, 2023 •

edited

Loading

Build stats

Test stats 🧪

pchila commented Jul 24, 2023

elasticmachine commented Jul 24, 2023 •

edited

Loading

elasticmachine commented Jul 24, 2023

blakerouse left a comment

pchila commented Jul 25, 2023

ycombinator Jul 25, 2023

pchila Jul 25, 2023 •

edited

Loading

cmacknz Jul 25, 2023

pchila Jul 26, 2023

ycombinator Jul 25, 2023

pchila Jul 25, 2023

blakerouse Jul 25, 2023

ycombinator Jul 25, 2023

ycombinator left a comment

pchila commented Jul 25, 2023

pchila commented Jul 26, 2023

pchila commented Jul 26, 2023

Pass go test flags to integration tests #3109

Pass go test flags to integration tests #3109

Conversation

pchila commented Jul 20, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

mergify bot commented Jul 20, 2023

elasticmachine commented Jul 20, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

pchila commented Jul 24, 2023

elasticmachine commented Jul 24, 2023 • edited Loading

🌐 Coverage report

elasticmachine commented Jul 24, 2023

blakerouse left a comment

Choose a reason for hiding this comment

pchila commented Jul 25, 2023

ycombinator Jul 25, 2023

Choose a reason for hiding this comment

pchila Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

cmacknz Jul 25, 2023

Choose a reason for hiding this comment

pchila Jul 26, 2023

Choose a reason for hiding this comment

ycombinator Jul 25, 2023

Choose a reason for hiding this comment

pchila Jul 25, 2023

Choose a reason for hiding this comment

blakerouse Jul 25, 2023

Choose a reason for hiding this comment

ycombinator Jul 25, 2023

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

pchila commented Jul 25, 2023

pchila commented Jul 26, 2023

pchila commented Jul 26, 2023

pchila commented Jul 20, 2023 •

edited

Loading

elasticmachine commented Jul 20, 2023 •

edited

Loading

elasticmachine commented Jul 24, 2023 •

edited

Loading

pchila Jul 25, 2023 •

edited

Loading