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

ESQL: Allow duplicate warnings in some tests #116199

Merged
merged 5 commits into from
Nov 6, 2024

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Nov 4, 2024

This fixes a test, actually in serverless Elasticsearch, that gets duplicate warnings. We'd like not to emit these duplicate warnings, but at this point it isn't worth it. So, for now, in some tests we allow duplicate warnings. In most of our tests we do not allow duplicate warnings so that we don't make more duplicate warnings without thinking about it.

This fixes a test, actually in serverless Elasticsearch, that gets
duplicate warnings. We'd like not to emit these duplicate warnings, but
at this point it isn't worth it. So, for now, in some tests we allow
duplicate warnings. In most of our tests we do not allow duplicate
warnings so that we don't make *more* duplicate warnings without
thinking about it.
@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL v9.0.0 v8.17.0 labels Nov 4, 2024
@nik9000 nik9000 requested a review from bpintea November 4, 2024 17:56
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Nov 4, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

Nice.

Comment on lines 151 to 154
if (deduplicateExact) {
return new AssertWarnings.DeduplicatedStrings(expectedWarnings);
}
return new AssertWarnings.ExactStrings(expectedWarnings);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the ternary operator would make this and below more legible to me, but it's fine this way too.

@nik9000 nik9000 added the auto-backport Automatically create backport pull requests when merged label Nov 6, 2024
@nik9000 nik9000 enabled auto-merge (squash) November 6, 2024 21:25
@nik9000 nik9000 merged commit dc59784 into elastic:main Nov 6, 2024
16 checks passed
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Nov 6, 2024
This fixes a test, actually in serverless Elasticsearch, that gets
duplicate warnings. We'd like not to emit these duplicate warnings, but
at this point it isn't worth it. So, for now, in some tests we allow
duplicate warnings. In most of our tests we do not allow duplicate
warnings so that we don't make *more* duplicate warnings without
thinking about it.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

elasticsearchmachine pushed a commit that referenced this pull request Nov 6, 2024
This fixes a test, actually in serverless Elasticsearch, that gets
duplicate warnings. We'd like not to emit these duplicate warnings, but
at this point it isn't worth it. So, for now, in some tests we allow
duplicate warnings. In most of our tests we do not allow duplicate
warnings so that we don't make *more* duplicate warnings without
thinking about it.
kderusso pushed a commit to kderusso/elasticsearch that referenced this pull request Nov 7, 2024
This fixes a test, actually in serverless Elasticsearch, that gets
duplicate warnings. We'd like not to emit these duplicate warnings, but
at this point it isn't worth it. So, for now, in some tests we allow
duplicate warnings. In most of our tests we do not allow duplicate
warnings so that we don't make *more* duplicate warnings without
thinking about it.
jozala pushed a commit that referenced this pull request Nov 13, 2024
This fixes a test, actually in serverless Elasticsearch, that gets
duplicate warnings. We'd like not to emit these duplicate warnings, but
at this point it isn't worth it. So, for now, in some tests we allow
duplicate warnings. In most of our tests we do not allow duplicate
warnings so that we don't make *more* duplicate warnings without
thinking about it.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
This fixes a test, actually in serverless Elasticsearch, that gets
duplicate warnings. We'd like not to emit these duplicate warnings, but
at this point it isn't worth it. So, for now, in some tests we allow
duplicate warnings. In most of our tests we do not allow duplicate
warnings so that we don't make *more* duplicate warnings without
thinking about it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants