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

Update elastic-package test command to define each command separately #1886

Merged
merged 11 commits into from
Jun 5, 2024

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Jun 4, 2024

Relates #787

This PR defines each subcommand under elastic-package test in its own function. Each test runner command now defines:

  • the minimum flags for them.
  • the needed processing depending on
    • whether or not the type of test can run per data stream.
    • whether or not test folders are required.
    • the package being an input or integration package.
  • the minimum fields in TestOptions struct.

This allows us to remove functions from the TestRunner interface: TestFolderRequired, CanRunPerDataStream, CanRunSetupTeardownIndependent

Take the opportunity in this PR to add the test type into the filename written for the xUnit results. This will help us to identify easily which XML file to select/open.

# before
build/test-results/
├── nginx_1717518654614473334.xml
├── nginx_1717518811869606562.xml
├── nginx_1717518815623075388.xml
└── nginx_1717518817400059691.xml

# after
build/test-results/
├── nginx_asset_1717517891228616986.xml
├── nginx_pipeline_1717517893039231373.xml
├── nginx_policy_1717517893079633874.xml
├── nginx_static_1717517893189605328.xml
└── nginx_system_1717518055889708880.xml

The current regex used in Junit step (Buildkite) keeps matching:

artifacts: "build/test-results/*.xml"

How to test

elastic-package stack up -v -d
cd /path/to/package

# test each test runner separately
elastic-package test asset -v
elastic-package test static -v
elastic-package test system -v
elastic-package test policy -v
elastic-package test pipeline -v

# test running all tests at once
elastic-package test -v

@mrodm mrodm self-assigned this Jun 4, 2024
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mrodm

@mrodm mrodm marked this pull request as ready for review June 4, 2024 17:14
@mrodm mrodm requested a review from a team June 4, 2024 17:14

func (r *runner) CanRunSetupTeardownIndependent() bool {
return false
}
Copy link
Member

Choose a reason for hiding this comment

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

👍 🔥

action := testTypeCommandActionFactory(runner)
testTypeCmdActions = append(testTypeCmdActions, action)
// Just used in pipeline and system tests
// Keep it here for backwards compatbility
Copy link
Member

Choose a reason for hiding this comment

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

As you prefer, I would be ok with removing this flag from other test runners if it is doing nothing there.


var results []testrunner.TestResult
for _, folder := range testFolders {
r, err := testrunner.Run(ctx, testType, testrunner.TestOptions{
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have specific commands for each test runner, maybe we can call here the specific runner, with an specific set of options? This would also allow to remove the registry of tests.

Actually now we wouldn't even need the test runner interface, right? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this could be possible, I'll check this out in a follow-up PR.

@mrodm mrodm merged commit 3f7eaaa into elastic:main Jun 5, 2024
3 checks passed
@mrodm mrodm deleted the set_testrunner_subcommands_independent branch June 5, 2024 08:12
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