Change Jest preset to "node" for server-side UTs #133017

gsoldevila · 2022-05-26T15:07:01Z

Summary

Part of the spacetime-511 initiative. We are updating Jest preset for server-side unit tests, switching from using a "browser-like" environment to a "node-like" one. This makes the testing environment match better the production one.

The idea is to switch the Jest preset for integration tests as well, but doing so breaks the build, as some tests remain hung awaiting for the logic to finish all async tasks. Thus, in order to have a graceful shutdown, we must first make sure there are no loose ends on the stop() lifecycle of the various services and plugins that shape Kibana.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2022-05-26T15:07:04Z

Pinging @elastic/kibana-core (Team:Core)

src/core/server/elasticsearch/elasticsearch_service.test.ts

spalger · 2022-05-26T17:38:03Z

src/core/server/elasticsearch/elasticsearch_service.test.ts

+      const subscription = setupContract.esNodesCompatibility$.subscribe(async () => {
+        expect(mockedClient.nodes.info).toHaveBeenCalledTimes(1);
+        await delay(10);
+        expect(mockedClient.nodes.info).toHaveBeenCalledTimes(2);
+
+        await elasticsearchService.stop();
+        await delay(100);
+        expect(mockedClient.nodes.info).toHaveBeenCalledTimes(2);
+        subscription.unsubscribe();
+        done();
+      });


The .subscribe(next) callback doesn't support promises, so the promise created by the async function passed to subscribe() will be ignored, triggering a unhandled promise rejection and process termination if these assertions ever fails To make sure that errors produced by expect() are propagated properly you should keep assertions in the observable using the proper operator. In this case, concatMap should be used because it will serially call the passed function for each value produced by setupContract.esNodesCompatibility$, wait for the promise to resolve, then produce the resolved value on the observable (undefined in this case because the async function has no return value), which will then trigger the firstValueFrom() helper and cause the observable to be unsubscribed.

Suggested change

const subscription = setupContract.esNodesCompatibility$.subscribe(async () => {

expect(mockedClient.nodes.info).toHaveBeenCalledTimes(1);

await delay(10);

expect(mockedClient.nodes.info).toHaveBeenCalledTimes(2);

await elasticsearchService.stop();

await delay(100);

expect(mockedClient.nodes.info).toHaveBeenCalledTimes(2);

subscription.unsubscribe();

done();

});

await firstValueFrom(

setupContract.esNodesCompatibility$.pipe(

Rx.concatMap(async () => {

expect(mockedClient.nodes.info).toHaveBeenCalledTimes(1);

await delay(10);

expect(mockedClient.nodes.info).toHaveBeenCalledTimes(2);

await elasticsearchService.stop();

await delay(100);

expect(mockedClient.nodes.info).toHaveBeenCalledTimes(2);

})

)

)

spalger · 2022-05-26T17:41:19Z

src/core/server/saved_objects/saved_objects_service.test.ts

-        done();
+
+      return new Promise<void>((done) => {
+        setImmediate(() => {


The timers/promises module has a promisified version of setImmediate you could use https://nodejs.org/api/timers.html#timerspromisessetimmediatevalue-options

Like my previous comment, since setImmediate() executes the callback in a separate callstack the error wouldn't propogate properly and would become unhandled, killing the process if this assertion ever failed so using the promisified version of setImmediate is actually important here.

pgayvallet

I'm probably missing the implications of changing the jest preset, so I have some questions:

pgayvallet · 2022-05-30T08:18:52Z

src/core/server/http/http_service.ts

+  private isListening(): boolean {
+    return this.httpServer?.isListening?.();
+  }


Why was this addition necessary (and why the optional chaining given this.httpServer is not optional)?

Alright, that's an interesting one. After some research I think I understood what's happening:

The following test in http_service_test.ts is failing as soon as we switch to the "node" preset:

test('register preboot route handler on setup', ...)

It turns out that this test is failing cause the mocked httpServer is not mocking the isListening method.
So, the fix is simple: we can add a new isListening: jest.fn() to the members of the mocked class.

The interesting question here is: Why was it NOT failing with the jsdom/browser-like preset?

Here goes my theory:
According to this GH thread:

RxJS catches the error thrown from the subscriber and rethrows it in the next tick.
This behavior has been in place since at least RxJs 6.0.0, I believe.
You can see this by waiting one tick yourself before finishing the test:

I believe

(A) with jest “jsdom” => all async ops are cancelled / aborted automatically when tests finish.

(B) with jest “node” => all async operations are awaited for before finishing the tests.

which makes the test (A) succeed, ignoring the RxJS error about to be reported, and gives test (B) enough time to fail.

TLDR: I will fix the test instead of changing prod code.

pgayvallet · 2022-05-30T08:21:47Z

src/core/server/elasticsearch/version_check/ensure_es_version.ts

+  return infoPromise
+    ? from(infoPromise).pipe(


Why this check? client.nodes.info always return a value. Are we adapting production code to adapt for testing environment rather than the other way around?

If I recall correctly, at some point during an integration test, the client.nodes.info() was returning undefined instead of a Promise. I don't remember the exact scenario but that's the reason I changed this. Perhaps it is linked to the fact that we start and stop servers very quickly for some tests, not giving this request enough time to complete.

I'll rollback this change to expose the error again.

UPDATE: No error anywhere 🤷🏼‍♂️ , so I guess we can discard the change!

pgayvallet · 2022-05-30T08:24:17Z

src/core/server/elasticsearch/elasticsearch_service.test.ts

  });
 });

 describe('#start', () => {
+  afterEach(async () => await elasticsearchService?.stop());


Should we put that at the top-level rather that in the start block?

Well, there are some #setup tests that don't even start() the ES service, and the #stop ones that are already calling stop(), so for those it is actually not needed.
I've just tried to move the stop() to the top-level as you suggest, and it does not do any harm. I'll update the code, this way we'll make sure we always call it.

kibana-ci · 2022-05-30T11:44:16Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #28

Test Failures

[job] [logs] FTR Configs #28 / lens app - group 1 lens smokescreen tests should allow to change index pattern

Metrics [docs]

✅ unchanged

History

💚 Build #47924 succeeded 3738f46
💚 Build #47872 succeeded a772de9

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

pgayvallet

LGTM

* change Jest preset to "node" for server-side UTs * small code enhancements, graceful stop (cherry picked from commit 0052fd2)

kibanamachine · 2022-05-31T08:40:12Z

💚 All backports created successfully

Status	Branch	Result
✅	8.3

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

* change Jest preset to "node" for server-side UTs * small code enhancements, graceful stop (cherry picked from commit 0052fd2) Co-authored-by: Gerard Soldevila <[email protected]>

change Jest preset to "node" for server-side UTs

c2a85f1

gsoldevila added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.3.0 labels May 26, 2022

gsoldevila requested review from a team as code owners May 26, 2022 15:07

spalger reviewed May 26, 2022

View reviewed changes

src/core/server/elasticsearch/elasticsearch_service.test.ts Outdated Show resolved Hide resolved

spalger approved these changes May 26, 2022

View reviewed changes

small code enhancements, graceful stop

a772de9

spalger reviewed May 26, 2022

View reviewed changes

Improve assertion failed handling for tests

3738f46

pgayvallet reviewed May 30, 2022

View reviewed changes

gsoldevila added 2 commits May 30, 2022 12:30

Rollback unnecessary changes

87abfde

Merge branch 'main' into spacetime-511-jest-node-for-unit-tests

3c86a32

gsoldevila requested a review from pgayvallet May 30, 2022 13:02

gsoldevila added auto-backport Deprecated - use backport:version if exact versions are needed v8.4.0 and removed backport:skip This commit does not require backporting labels May 31, 2022

pgayvallet approved these changes May 31, 2022

View reviewed changes

gsoldevila merged commit 0052fd2 into elastic:main May 31, 2022

kibanamachine mentioned this pull request May 31, 2022

[8.3] Change Jest preset to "node" for server-side UTs (#133017) #133179

Merged

kibanamachine pushed a commit that referenced this pull request May 31, 2022

Change Jest preset to "node" for server-side UTs (#133017)

5e8596c

* change Jest preset to "node" for server-side UTs * small code enhancements, graceful stop (cherry picked from commit 0052fd2)

TinaHeiligers mentioned this pull request Jul 19, 2022

Migrate server-side ES domain to packages #136297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Jest preset to "node" for server-side UTs #133017

Change Jest preset to "node" for server-side UTs #133017

gsoldevila commented May 26, 2022 •

edited

Loading

elasticmachine commented May 26, 2022

spalger May 26, 2022 •

edited

Loading

gsoldevila May 26, 2022

spalger May 26, 2022 •

edited

Loading

pgayvallet left a comment

pgayvallet May 30, 2022 •

edited

Loading

gsoldevila May 30, 2022

pgayvallet May 30, 2022 •

edited

Loading

gsoldevila May 30, 2022

gsoldevila May 30, 2022

pgayvallet May 30, 2022 •

edited

Loading

gsoldevila May 30, 2022 •

edited

Loading

kibana-ci commented May 30, 2022

pgayvallet left a comment

kibanamachine commented May 31, 2022

Change Jest preset to "node" for server-side UTs #133017

Change Jest preset to "node" for server-side UTs #133017

Conversation

gsoldevila commented May 26, 2022 • edited Loading

Summary

Checklist

elasticmachine commented May 26, 2022

spalger May 26, 2022 • edited Loading

Choose a reason for hiding this comment

gsoldevila May 26, 2022

Choose a reason for hiding this comment

spalger May 26, 2022 • edited Loading

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet May 30, 2022 • edited Loading

Choose a reason for hiding this comment

gsoldevila May 30, 2022

Choose a reason for hiding this comment

pgayvallet May 30, 2022 • edited Loading

Choose a reason for hiding this comment

gsoldevila May 30, 2022

Choose a reason for hiding this comment

gsoldevila May 30, 2022

Choose a reason for hiding this comment

pgayvallet May 30, 2022 • edited Loading

Choose a reason for hiding this comment

gsoldevila May 30, 2022 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented May 30, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

pgayvallet left a comment

Choose a reason for hiding this comment

kibanamachine commented May 31, 2022

💚 All backports created successfully

Questions ?

gsoldevila commented May 26, 2022 •

edited

Loading

spalger May 26, 2022 •

edited

Loading

spalger May 26, 2022 •

edited

Loading

pgayvallet May 30, 2022 •

edited

Loading

pgayvallet May 30, 2022 •

edited

Loading

pgayvallet May 30, 2022 •

edited

Loading

gsoldevila May 30, 2022 •

edited

Loading