Skip to content

EQL: EqlActionIT improvements#53780

Merged
aleksmaus merged 3 commits intoelastic:masterfrom
aleksmaus:issue/53598_eqlactionit_test_improvements
Mar 20, 2020
Merged

EQL: EqlActionIT improvements#53780
aleksmaus merged 3 commits intoelastic:masterfrom
aleksmaus:issue/53598_eqlactionit_test_improvements

Conversation

@aleksmaus
Copy link
Copy Markdown
Contributor

Move EqlActionIT to EQL QA project.
Use HL client for EQL verification harness.
Improve the speed of tests, setting the data once and reusing for each parameterized test.
Improve naming for the tests, using "description" or "note" and falling back to "query".

Related to #53598

@aleksmaus aleksmaus requested review from astefan, costin and imotov March 19, 2020 03:22
Copy link
Copy Markdown
Contributor Author

@aleksmaus aleksmaus Mar 19, 2020

Choose a reason for hiding this comment

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

@costin I was not sure what to put in here, but now looks like I need these files for jtoml dependency verification task in EQL QA project.
let me know.

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.

If the library doesn't have a NOTICE file, then I don't think you need to add one.

Copy link
Copy Markdown
Contributor Author

@aleksmaus aleksmaus Mar 19, 2020

Choose a reason for hiding this comment

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

The build requires this file, at least an empty one :-) Can remove the content.

@aleksmaus aleksmaus force-pushed the issue/53598_eqlactionit_test_improvements branch from d39adc5 to a592b9a Compare March 19, 2020 03:47
@aleksmaus aleksmaus added the :Analytics/EQL EQL querying label Mar 19, 2020
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search (:Search/EQL)

@aleksmaus aleksmaus added the >test Issues or PRs that are addressing/adding tests label Mar 19, 2020
Copy link
Copy Markdown
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

Left some comments

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.

Simply add the license from the jar - no need to modify it to add copyright or something else.

Copy link
Copy Markdown
Contributor Author

@aleksmaus aleksmaus Mar 19, 2020

Choose a reason for hiding this comment

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

jar has only this blob in pom.xml
<licenses> <license> <name>The Apache Software License, Version 2.0</name> <url>http://www.apache.org/licenses/LICENSE-2.0.txt</url> <distribution>repo</distribution> </license> </licenses>
should I just copy the http://www.apache.org/licenses/LICENSE-2.0.txt content instead there?

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.

Updated the jtoml-LICENSE file with the license content. Removed the NOTICE file content, but the file needs to be present to pass all the thridparty deps validations here.

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.

If the library doesn't have a NOTICE file, then I don't think you need to add one.

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.

Why synchronized?

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.

The client in the ESRestTestCase base class is created on @before, so this is done in order to get the initialized client but still synchronize the setup and do it once in case the tests are run in parallel.

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.

You could use the approach used in SQL:

, that is on each test check whether the index exists - if it doesn't load the data, otherwise ignore it.

And wipe the data in a @AfterClass. Note that you could do the loading inside @BeforeClass as well if there are no modifications to the index at all.

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.

The client from the base class only available on after @before hook. Unless I hack around that, the safest bet is to initialize on @before and since I need to initialize it once that's why synchronizing on the class

@aleksmaus aleksmaus force-pushed the issue/53598_eqlactionit_test_improvements branch from a592b9a to c8a4cc0 Compare March 19, 2020 17:48
@aleksmaus
Copy link
Copy Markdown
Contributor Author

Rebased against the latest master to resolve the conflicts and force pushed.

Copy link
Copy Markdown
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM w/o the synchronized blocks.

@aleksmaus aleksmaus merged commit 065fc15 into elastic:master Mar 20, 2020
aleksmaus added a commit to aleksmaus/elasticsearch that referenced this pull request Mar 20, 2020
aleksmaus added a commit that referenced this pull request Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/EQL EQL querying >test Issues or PRs that are addressing/adding tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants