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

[test] use randomized runner in packaging tests #32109

Merged

Conversation

andyb-elastic
Copy link
Contributor

Use the randomized runner from the test framework and add some basic
logging to make the packaging tests behave more similarly to how we use
junit in the rest of the project

Use the randomized runner from the test framework and add some basic
logging to make the packaging tests behave more similarly to how we use
junit in the rest of the project
@andyb-elastic andyb-elastic added >test Issues or PRs that are addressing/adding tests :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v7.0.0 v6.4.0 labels Jul 16, 2018
@andyb-elastic andyb-elastic requested review from rjernst and alpar-t July 16, 2018 23:37
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

protected final Log logger = LogFactory.getLog(getClass());

@Rule
public final TestName testNameRule = new TestName();
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 reason for creating this class and replicating some of the stuff in ESTestCase instead of using it directly is that there are some things that ESTestCase does (e.g. bootstrap checks) that don't play well with how we run these tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we extend ESTestCase and disable what we don't need or doesn't work ?
Some things might still be good to have here, I'm thinking about especially the reproduction line,
but I guess that wouldn't really work either because of how the order of test methods is inter-dependent.
What is the advantage o f having ti like that ? Would it make sense to bring everything that needs to run in order under a single method ? Then we could run in truly random fashion and perhaps keep more functionality from the test framework. As it is I'm not fully convinced that it's worth adding it just to throw out most of what it does and without really making use of any randomness in these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have made this more clear - the real purpose of doing this is so that these tests will read similarly to the other junit tests wrt method names and such. There isn't as far as I can tell a way to configure that with any vanilla junit runners

There is no randomization here because the method ordering matters. The order of the test classes (not methods) could be randomized, but I don't think randomized runner does that and I didn't figure out a way to have it handle the suite itself rather than using @RunWith(Suite.class)

I spent some time on making it work with ESTestCase a while ago the first time I tried this and disabling/working around/making configurable the things in there that don't play nice with these tests was nontrivial as I remember, and I'm not sure I was really successful. It seemed much more straightforward to do it with its own parent test case

Currently we get a repro line from the vagrant test plugin

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, using ESTestCase means bringing in elasticsearch as a dependency, which doesn't really make sense from a packaging test perspective (we won't make use of most of the test utilities provided there). We currently have elasticsearch as a dependency for the rest test runner, but it would be good to split that out so we can avoid the dependencies that ES pulls in (and can conflict with things we would like to do in the rest test runner, eg using the rest client).

Copy link
Contributor

Choose a reason for hiding this comment

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

You could look at GradleUnitTestCase it does the same by pulling int the randomized runner only.

What I was wondering about w.r.t order is that if it really makes sense to have it fixed. If all we are doing is going trough methods sequentially what advantage does it bring to have them in separate methods ? Maybe better error reporting ? Should we keep the randomized method order and make sure it actually works like that? I'm not saying we need to change it just looking to understand the implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, the benefit of the method ordering is mostly just to make it a little easier to read where something went wrong. And to separate out parts that aren't directly related to each other so it doesn't end up as just one very long test

I'd ideally like to randomize them and reinstall the distribution before each one so it starts from a predictable state (and some of them definitely can work like this already) but it would add too much runtime to an already very long test suite

@andyb-elastic
Copy link
Contributor Author

This will need to be updated and merged after #31943 is merged so that the test cases it adds can be updated too - the changes from there will be more of the same

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I have a couple comments.


compile "org.apache.httpcomponents:httpcore:${versions.httpcore}"
compile "org.apache.httpcomponents:httpclient:${versions.httpclient}"
compile "org.apache.httpcomponents:fluent-hc:${versions.httpclient}"
compile "commons-codec:commons-codec:${versions.commonscodec}"
compile "commons-logging:commons-logging:${versions.commonslogging}"

compile "org.elasticsearch.test:framework:${version}"
Copy link
Member

Choose a reason for hiding this comment

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

I think you could pull in randomized runner on its own? Additionally, there is a JUnit3MethodProvider in randomized runner, can we use that instead of the lucene one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I changed it back to use randomizedtesting instead of the framework - I'd added it when I was trying to make it work with ESTestCase


@Before
public void logTestNameBefore() {
logger.info("[" + testNameRule.getMethodName() + "]: before test");
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? Seems like it will produce a lot of noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want it because it'll be necessary to make sense of any other logging we do in the tests. The noise won't show up for people running it in the console because of how the vagrant tasks moderate their output - it'll only show up in CI because it runs with --console plain --info

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a before and after? These are run completely sequentially, so the "before" of one test is the "after" of the previous. I'm just thinking of what the old output used to look like (a single line per test in most cases with "OK") compared to what we are moving to here (many lines per test, if I understand correctly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I removed the after test logging

@andyb-elastic
Copy link
Contributor Author

Jenkins run gradle build tests please

@andyb-elastic andyb-elastic merged commit e20f59a into elastic:master Jul 18, 2018
andyb-elastic added a commit to andyb-elastic/elasticsearch that referenced this pull request Jul 18, 2018
Use the randomized runner from the test framework and add some basic
logging to make the packaging tests behave more similarly to how we use
junit in the rest of the project
andyb-elastic added a commit that referenced this pull request Jul 18, 2018
Use the randomized runner from the test framework and add some basic
logging to make the packaging tests behave more similarly to how we use
junit in the rest of the project
dnhatn added a commit that referenced this pull request Jul 19, 2018
* 6.x:
  Fix rollup on date fields that don't support epoch_millis (#31890)
  Revert "Introduce a Hashing Processor (#31087)" (#32179)
  [test] use randomized runner in packaging tests (#32109)
  Painless: Fix caching bug and clean up addPainlessClass. (#32142)
  Fix BwC Tests looking for UUID Pre 6.4 (#32158) (#32169)
  Call setReferences() on custom referring tokenfilters in _analyze (#32157)
  Add more contexts to painless execute api (#30511)
  Add EC2 credential test for repository-s3 (#31918)
  Fix CP for namingConventions when gradle home has spaces (#31914)
  Convert Version to Java - clusterformation part1 (#32009)
  Fix Java 11 javadoc compile problem
  Improve docs for search preferences (#32098)
  Configurable password hashing algorithm/cost(#31234) (#32092)
  [DOCS] Update TLS on Docker for 6.3
  ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are
  Switch distribution to new style Requests (#30595)
  Build: Skip jar tests if jar disabled
  Build: Move shadow customizations into common code (#32014)
  Painless: Add PainlessClassBuilder (#32141)
  Fix accidental duplication of bwc test for script behavior
  Handle missing values in painless (#30975) (#31903)
  Build: Make additional test deps of check (#32015)
  Painless: Fix Bug with Duplicate PainlessClasses (#32110)
  Adjust translog after versionType removed in 7.0 (#32020)
  Disable C2 from using AVX-512 on JDK 10 (#32138)
  [Rollup] Add new capabilities endpoint for concrete rollup indices (#32111)
  Mute :qa:mixed-cluster indices.stats/10_index/Index - all’
  [ML] Wait for aliases in multi-node tests (#32086)
  Ensure to release translog snapshot in primary-replica resync (#32045)
  Docs: Fix missing example script quote (#32010)
  Add Index UUID to `/_stats` Response (#31871) (#32113)
  [ML] Move analyzer dependencies out of categorization config (#32123)
  [ML][DOCS] Add missing 6.3.0 release notes (#32099)
  Updates the build to gradle 4.9 (#32087)
  Update monitoring template version to 6040099 (#32088)
  Fix put mappings java API documentation (#31955)
  Add exclusion option to `keep_types` token filter (#32012)
dnhatn added a commit that referenced this pull request Jul 20, 2018
* master:
  Painless: Simplify Naming in Lookup Package (#32177)
  Handle missing values in painless (#32207)
  add support for write index resolution when creating/updating documents (#31520)
  ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864)
  Remove indication of future multi-homing support (#32187)
  Rest test - allow for snapshots to take 0 milliseconds
  Make x-pack-core generate a pom file
  Rest HL client: Add put watch action (#32026)
  Build: Remove pom generation for plugin zip files (#32180)
  Fix comments causing errors with Java 11
  Fix rollup on date fields that don't support epoch_millis (#31890)
  Detect and prevent configuration that triggers a Gradle bug (#31912)
  [test] port linux package packaging tests (#31943)
  Revert "Introduce a Hashing Processor (#31087)" (#32178)
  Remove empty @return from JavaDoc
  Adjust SSLDriver behavior for JDK11 changes (#32145)
  [test] use randomized runner in packaging tests (#32109)
  Add support for field aliases. (#32172)
  Painless: Fix caching bug and clean up addPainlessClass. (#32142)
  Call setReferences() on custom referring tokenfilters in _analyze (#32157)
  Fix BwC Tests looking for UUID Pre 6.4 (#32158)
  Improve docs for search preferences (#32159)
  use before instead of onOrBefore
  Add more contexts to painless execute api (#30511)
  Add EC2 credential test for repository-s3 (#31918)
  A replica can be promoted and started in one cluster state update (#32042)
  Fix Java 11 javadoc compile problem
  Fix CP for namingConventions when gradle home has spaces (#31914)
  Fix `range` queries on `_type` field for singe type indices (#31756)
  [DOCS] Update TLS on Docker for 6.3 (#32114)
  ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are
  Remove versionType from translog (#31945)
  Switch distribution to new style Requests (#30595)
  Build: Skip jar tests if jar disabled
  Painless: Add PainlessClassBuilder (#32141)
  Build: Make additional test deps of check (#32015)
  Disable C2 from using AVX-512 on JDK 10 (#32138)
  Build: Move shadow customizations into common code (#32014)
  Painless: Fix Bug with Duplicate PainlessClasses (#32110)
  Remove empty @param from Javadoc
  Re-disable packaging tests on suse boxes
  Docs: Fix missing example script quote (#32010)
  [ML] Wait for aliases in multi-node tests (#32086)
  [ML] Move analyzer dependencies out of categorization config (#32123)
  Ensure to release translog snapshot in primary-replica resync (#32045)
  Handle TokenizerFactory  TODOs (#32063)
  Relax TermVectors API to work with textual fields other than TextFieldType (#31915)
  Updates the build to gradle 4.9 (#32087)
  Mute :qa:mixed-cluster indices.stats/10_index/Index - all’
  Check that client methods match API defined in the REST spec (#31825)
  Enable testing in FIPS140 JVM (#31666)
  Fix put mappings java API documentation (#31955)
  Add exclusion option to `keep_types` token filter (#32012)
  [Test] Modify assert statement for ssl handshake (#32072)
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants