Converting Derivative Pipeline Agg integration test into AggregatorTestsCase.

[mirkojotic]

This is a WIP PR for this conversion as a part of #36015

I tried not modifying any tests but just change the underlying setup. That being said I did hit a couple of snags.

testPartiallyUnmapped method seems to be using two indices. I wasn't sure how to handle that so I'm open to any suggestions.

In testAvgMovavgDerivNPE I tried using NumericDocValuesField for "value" field but since it's sometimes null it would throw an NPE. Again any advice is welcome :)

Anywhere in the file where number of hits is referenced I would loop over buckets and get doc count. Not sure if proper approach.

If I made some more errors or if something can be improved I'm always open to suggestions or direction.

@polyfractal

[elasticmachine]

Pinging @elastic/es-analytics-geo

[polyfractal]

Awesome, thanks for working on this!

testPartiallyUnmapped method seems to be using two indices. I wasn't sure how to handle that so I'm open to any suggestions.

Hmm, yeah, I'm not sure if we'll be able to replicate that at the moment. We'd have to modify AggregatorTestCase to accept multiple IndexSearcher, then generate and merge multiple ShardSearcher's. Pretty outside the scope of this PR, and probably not worth the hassle.

I'd say 👍 to just skipping the partially-unmapped test.

In testAvgMovavgDerivNPE I tried using NumericDocValuesField for "value" field but since it's sometimes null it would throw an NPE. Again any advice is welcome :)

Was the NPE when adding the null value, or when testing?

I think you can do document.add(new NumericDocValuesField("value", null)); to add it, the Document will accept a Long (which allows it being null).

Anywhere in the file where number of hits is referenced I would loop over buckets and get doc count. Not sure if proper approach.

Ahh, yeah, that works :) We don't have access to the total hit count easily in these tests, so that's a good proxy. Word of warning though, I think it works for these tests but it could be fragile elsewhere. A document is allowed to be added to multiple buckets (e.g. overlapping ranges, multiple values that each land in a different bucket, etc) so doc_count doesn't always equal the total hits.

I think that's an assertion we can drop if it becomes flaky, although from skimming the tests I think it will be fine here.

I didn't look suuuper closely at all the tests yet, but from a skim I think they look good! Thanks for doing these! Once we've resolved the NPE test I'll kick off a build and give all the tests a closer look. :)

[mirkojotic]

No problem. It wasn't a difficult process as I followed what you did for the initial one 😊

I think you can do document.add(new NumericDocValuesField("value", null)); to add it, the Document will accept a Long (which allows it being null).

This is what I tried initially and it would throw a NPE. But let me go back and re-check it and report back.

Edit:

java.lang.IllegalArgumentException: field="value": null value not allowed

when doing

document.add(new NumericDocValuesField("value", null));

I don't know if I could be doing something wrong but that error is pretty conclusive.
@polyfractal

[polyfractal]

Hm gotcha, I'll take a closer look today. The alternative is to just skip the field in that document (which is essentially equivalent), but I think there's already a test that skips. I'll go digging to see the history of that NPE test, it looks like something that was added for a specific issue. If it only pertains to XContent parsing or something we can probably ignore it.

[mirkojotic]

Sounds good. Let me know if you want me to look into the history of it.

And my current solution just skips indexing that field if it's null. Which is only twice.

[polyfractal]

I had a quick look and it seems to originate from 39a067a, and is basically just testing that the derivative can handle missing/skipped values. So I think the approach of just omitting it from the document is fine here, which is equivalent to setting null in the JSON (e.g. we just don't add nulls to documents).

Think this is ready for CI test runs in that case?

[mirkojotic]

Great. In that case I believe it is.

[polyfractal]

👍

@elasticmachine test this please

[mirkojotic]

I have code style issues. I completely overlooked that. I'm sorry.

I'll make sure that's fixed and let you know.

[mirkojotic]

@polyfractal I've fixed the style issues and gradle checkStyle is successful. So I think it's ready for another CI run.

One problem I had is that gradle check isn't really running well for me and it never was. It doesn't finish successfully and I'm not sure why. I made a docker container where I have jdk 11 and gradle 5.2.1 and still no luck. Maybe because I'm missing JAVA8_HOME, JAVA9_HOME etc.

[Blackiie]

Aún no entiendo esto muy bien pero, aquí estoy asiendo presencia y trataré de seguirlo asiendo 😀. Gracias!! El 12 feb. 2019 19:09, "Mirko Jotic" <[email protected]> escribió:

…

@polyfractal <https://github.com/polyfractal> I've fixed the style issues and gradle checkStyle is successful. So I think it's ready for another CI run. One problem I had is that gradle check isn't really running well for me and it never was. It doesn't finish successfully and I'm not sure why. I made a docker container where I have jdk 11 and gradle 5.2.1 and still no luck. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#38679 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AtTlE4iFio7Rq69iEPCuHslBZqp8aUVCks5vM4HLgaJpZM4azeBB> .

[polyfractal]

@elasticmachine test this please

@mirkojotic Hmm, what sort of errors are you getting? Are you invoking it with the gradle wrapper (gradlew instead of gradle)?

If you're just seeing test failures, and not something that looks like the gradle build itself is broken... it could just be unrelated failures. We're actually in a team-wide effort at the moment to fix flaky/broken tests since the spurious failure rate has creeped up over time, so you may have run into an unrelated broken test.

Also as a tip for running checkstyle stuff fast, you can do ./gradlew precommit --parallel which will speed it up a bit :) Parallel test runs unfortunately have issues at the moment, so --parallel won't work for check, test, etc.

[mirkojotic]

Well it failed again. I looked at Jenkins console of the first CI and couldn't discern what went wrong. I'm willing to work on it whatever it is so any help with figuring out the cause is appreciated.

[andyb-elastic]

@mirkojotic the latest CI failures should be resolved by merging master back into your branch (there were some changes to the build since your branch was created)

https://scans.gradle.com/s/5nbkdc76vfpk2/console-log?task=:distribution:bwc:minor:checkoutBwcBranch

:distribution:bwc:minor:checkoutBwcBranch FAILED
--
Checking out elasticsearch elastic/6.x for branch 6.x
error: pathspec 'elastic/6.x' did not match any file(s) known to git.

[mirkojotic]

@andyb-elastic I did a quick fetch of upstream, merge into feature branch. Hope that's what you meant.

Since you asked @polyfractal below is one of the errors I'm getting. There is around 30 and all very similar NoClassDefFoundError. And yes I'm using gradlew.

Suite: org.elasticsearch.test.VersionUtilsTests
ERROR   0.00s J1 | VersionUtilsTests (suite) <<< FAILURES!
   > Throwable #1: java.lang.NoClassDefFoundError: Could not initialize class org.elasticsearch.test.ESTestCase
   >    at java.base/java.lang.Class.forName0(Native Method)
   >    at java.base/java.lang.Class.forName(Class.java:398)
Completed [24/36] on J1 in 0.03s, 0 tests, 1 error <<< FAILURES!

Anyhow It might be ready for another CI run.

[andyb-elastic]

@elasticmachine test this please

@mirkojotic yup that's what I meant. I checked out your branch and I'm not seeing that issue with NoClassDefFoundError in my environment

You've probably seen this already, but here's our high level guidelines on setting up your environment for our build. Some people find they have to set the relevant JAVA_HOME variables in their shell profile rather than just in the shell they're running gradlew from, due to how gradle works

I'm assuming you're running gradlew from the shell here, when seeing problems that look like issues with the build, I usually run a git -xdf clean to clean out anything that may be lingering from previous builds (this will delete anything that's not committed)

[mirkojotic]

Well, some progress has been made 😊

@andyb-elastic I didn't even think that there's something wrong with the tests, is definitely my environment. I'll try out your suggestion.

Edit
After working on and fixing my JDK issues (I had two JDKs so I made sure to use the same one) ./gradlew check seems to work fine and everything passes except for tests requiring java versions other than 11. Thanks Andy!!!

That being said this issue in CI 2 I do not understand.

[polyfractal]

I think that CI 2 failure is unrelated, alas. :(

@elasticmachine run elasticsearch-ci/2

[mirkojotic]

Hurray! I'll be watching for comments.

Thank you both for being patient!

[polyfractal]

Merging, and will backport to 7.x as well. Thanks @mirkojotic!

[polyfractal]

Also a note: I figure we can let these churn away on CI for a little bit, then remove the duplicated parts from the existing integration tests once we think it's stable. :)

[mirkojotic]

@polyfractal awesome! Happy to hear it!

I'll get to work on some other ones if it makes sense.