Skip to content

Conversation

@nik9000
Copy link
Member

@nik9000 nik9000 commented Feb 23, 2021

Revamps the integration tests for the filter agg to be more clear and
builds integration tests for the fitlers agg. Both of these
integration tests are fairly basic but they do assert that the aggs
work.

@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v8.0.0 v7.13.0 labels Feb 23, 2021
@nik9000 nik9000 requested a review from polyfractal February 23, 2021 13:55
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 23, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@nik9000
Copy link
Member Author

nik9000 commented Feb 23, 2021

Looks like we already covered a bunch of this in #27128. I'll see what I can merge.

Revamps the integration tests for the `filter` agg to be more clear and
builds integration tests for the `fitlers` agg. Both of these
integration tests are fairly basic but they do assert that the aggs
work.
@nik9000
Copy link
Member Author

nik9000 commented Feb 23, 2021

Looks like we already covered a bunch of this in #27128. I'll see what I can merge.

I've merged the cache hitting tests into the filters_buckets yml. That feels useful. And so does the cleanup for fitler (singular) agg.

@nik9000 nik9000 requested review from imotov and removed request for polyfractal April 12, 2021 17:18
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

Could you resolve conflicts and apply suggestions in #69440?

Copy link
Member Author

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

@imotov done! Thanks for reviewing!


//there is something wrong when using dotted document syntax here, passes in main yaml tests
'search/330_fetch_fields/Test nested field inside object structure',
'search/330_fetch_fields/Test nested field inside object structure',
Copy link
Member Author

Choose a reason for hiding this comment

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

Was a tab I think.

@nik9000
Copy link
Member Author

nik9000 commented Apr 14, 2021

I've tracked down the caching error. As of 10 days ago the version of the node that received the request is part of the cache key. If we're going to assert that we cache we'll need to be careful. I'll push an update in a bit.

@nik9000
Copy link
Member Author

nik9000 commented Apr 14, 2021

@imotov I believe this is ready for another round. The tests that were failing for me are are passing locally for me now. Let's see what Jenkins says about 'em.

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM

throw new XContentParseException(parser.getTokenLocation(), "expected [version] to be a value");
}
List<VersionRange> skipVersionRanges = SkipSection.parseVersionRanges(parser.text());
List<VersionRange> skipVersionRanges = parser.text().equals("current")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like current here, it was pretty confusing when I looked at it for the first time, but I cannot come up with anything better, and more I look at it, the more it makes sense 😄. I tried latest and same but they seem to be even more confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Version.CURRENT is the name of the constant. But, yeah, I get it. In the yaml context its weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to merge this with current and hope that we can come up with a better name soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes perfect sense in Version, since there is only one version that we are currently working with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm with you that it is weird. But all the alternatives I can think of are confusing too. latest, same, this_branch.

@nik9000 nik9000 merged commit 2d6f8d1 into elastic:master Apr 14, 2021
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Apr 14, 2021
Revamps the integration tests for the `filter` agg to be more clear and
builds integration tests for the `fitlers` agg. Both of these
integration tests are fairly basic but they do assert that the aggs
work.
mark-vieira pushed a commit that referenced this pull request Apr 15, 2021
Revamps the integration tests for the `filter` agg to be more clear and
builds integration tests for the `fitlers` agg. Both of these
integration tests are fairly basic but they do assert that the aggs
work.
nik9000 added a commit that referenced this pull request Apr 15, 2021
Revamps the integration tests for the `filter` agg to be more clear and
builds integration tests for the `fitlers` agg. Both of these
integration tests are fairly basic but they do assert that the aggs
work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v7.13.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants