Skip to content

[ES|QL] Validation tests agains Elasticsearch #270445

Open
bartoval wants to merge 21 commits into
elastic:mainfrom
bartoval:validation-tests-es
Open

[ES|QL] Validation tests agains Elasticsearch #270445
bartoval wants to merge 21 commits into
elastic:mainfrom
bartoval:validation-tests-es

Conversation

@bartoval
Copy link
Copy Markdown
Contributor

@bartoval bartoval commented May 21, 2026

Summary

Part of #218936

I kept the logic intentionally basic— there’s no file autodetection or other “smart” behavior.

To test it locally:

yarn test:jest_integration --config src/platform/packages/shared/kbn-esql-language/jest.integration.config.js
buildkite

@bartoval bartoval self-assigned this May 21, 2026
@bartoval bartoval added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana t// v9.5.0 labels May 21, 2026
@bartoval
Copy link
Copy Markdown
Contributor Author

/ci

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • 🛠️ Update Documentation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

1 similar comment
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • 🛠️ Update Documentation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@bartoval bartoval changed the title [ES|QL] validation tests agains Elaticsearch [ES|QL] Validation tests agains Elaticsearch May 21, 2026
@bartoval bartoval changed the title [ES|QL] Validation tests agains Elaticsearch [ES|QL] Validation tests agains Elasticsearch May 21, 2026
@bartoval bartoval force-pushed the validation-tests-es branch from a79793e to 1b3af55 Compare May 21, 2026 16:00
@bartoval
Copy link
Copy Markdown
Contributor Author

/ci

@bartoval
Copy link
Copy Markdown
Contributor Author

/ci

@bartoval
Copy link
Copy Markdown
Contributor Author

/ci

@bartoval
Copy link
Copy Markdown
Contributor Author

/ci

@bartoval bartoval force-pushed the validation-tests-es branch from 9839573 to 6e8e64c Compare May 27, 2026 15:42
@bartoval
Copy link
Copy Markdown
Contributor Author

/ci

@bartoval
Copy link
Copy Markdown
Contributor Author

bartoval commented May 28, 2026

I'm not closing this task yet because I still have a couple of open points to clarify:

  • src/platform/packages/shared/kbn-esql-language/src/language/validation/validation.test.ts is currently a monolith that generates the JSON consumed by the FRT tests. In theory, this behavior is already covered by FRT, but if the intention is to remove those FRT tests, then this coverage needs to be added here. This opens up a couple more modifcations that I will eventually address in another pr.

  • Right now, we cover false positives — cases where Kibana reports an error but Elasticsearch actually accepts the query, effectively blocking the user unnecessarily.
    I think it makes sense not to cover false negatives -> The user can execute the query and will receive the error directly from Elasticsearch.

  • I’m using the default timeout preset, which is jest.setTimeout(10 * 60 * 1000) (10 minutes). If we want a lower timeout for this test, I’ll need to set it explicitly.
    src/platform/packages/shared/kbn-test/jest_integration_node/jest-preset.js -> src/platform/packages/shared/kbn-test/src/jest/setup/after_env.integration.js

  • I created these suites so they can be reused for both integration and unit tests, mainly for convenience since the set of files is more or less fixed and doesn’t really change over time. When a new file is added, it just needs to be imported manually into the suite. Since that’s a minimal effort, I preferred to avoid adding extra abstractions unnecessarily.

@bartoval bartoval marked this pull request as ready for review May 28, 2026 07:02
@bartoval bartoval requested a review from a team as a code owner May 28, 2026 07:02
@infra-vault-gh-plugin-prod
Copy link
Copy Markdown

Pinging @elastic/kibana-esql (Team:ESQL)

Copy link
Copy Markdown
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

@bartoval we also have these src/platform/test/api_integration/apis/esql so having some there and the other here is a bit confusing.

I suggest to have one place to put the integration tests and a common testing strategy. Can you take a look holistically?

@bartoval bartoval requested a review from a team as a code owner May 29, 2026 11:07
@bartoval
Copy link
Copy Markdown
Contributor Author

@bartoval we also have these src/platform/test/api_integration/apis/esql so having some there and the other here is a bit confusing.

I suggest to have one place to put the integration tests and a common testing strategy. Can you take a look holistically?

  • I removed the FRTs and the JSON file generated from validation.test.ts for these tests.
  • I partially migrated validation.test.ts into dedicated test files. This way, the tests follow the same strategy. I already did this as a pilot for sources and command.
  • I’ll migrate the rest in another PR because handling the whole migration here would become difficult.

Copy link
Copy Markdown
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

I like where it goes but I would like us to be more strategic:

  • we need to optimize to not make CI slower

await esqlEnv.setupIndicesPolicies();
});

afterAll(async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

 afterAll(async () => {
    try {
      await esqlEnv?.cleanup();
    } finally {
      await esqlEnv?.integrationEnv.stop();
    }
  });

if (type === 'cartesian_shape') {
esType = 'shape';
}
if (type === 'aggregate_metric_double') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

{ ignore: [404] }
);
for (const policy of policies) {
await es.enrich.deletePolicy({ name: policy.name }, { ignore: [404] });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are deleting the policies but not the backing indices.

// Integration suites use this hook to compare the same validation run with ES.
// Unit suites leave it undefined, so their behavior stays unchanged.
const promise = validateQuery(query, cb).then(async (result) => {
await afterValidate?.({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to do some optimizations before merging this Val. With all the efforts done to improve the CI time I am afraid that having these run in every PR might make the situation worse especially when we run this sequentially.

Some ideas:

  • Batch the ES queries
  • Only check queries with non-syntax errors

@kibanamachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Scout Lane #11 - stateful-classic / default / local-stateful-classic - Saved query menu — CRUD (Discover) - save, load, update, save-as-new, delete via the popover
  • [job] [logs] Scout Lane #12 - stateful-classic / default / local-stateful-classic - UptimeIntegrationDeprecation - returns true when non-managed synthetics policies exist

Metrics [docs]

✅ unchanged

History

cc @bartoval

@bartoval
Copy link
Copy Markdown
Contributor Author

bartoval commented Jun 1, 2026

  • I don't cover the false negative => The user can execute the query and will receive the error directly from Elasticsearch.
  • One reason I didn't optimize is that test ran fast. Where did you see any slowdowns?

@stratoula
Copy link
Copy Markdown
Contributor

stratoula commented Jun 1, 2026

  • The user can execute the query and will receive the error directly from Elasticsearch.

My bad Valerio, I got confused from one comment and thought we are not doing this

Where did you see any slowdowns?

I didnt run them, from the code review it seems as doing this sequentially will make CI slower (?) So if we could batch them and make it even faster would be great. Unless the @elastic/appex-qa is ok with it. Not sure which is the process here but I wouldnt want to have complains about us making the CI slower in each PR 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:ESQL ES|QL related features in Kibana t// v9.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants