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

Prepare OSS unit tests for jest-circus #82280

Closed

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Nov 2, 2020

Summary

Preparation work for:
#77894

This PR enables by default jest-circus as the main test runner for our OSS jest tests.
Unfortunately angular-mocks doesn't support env other than mocha or jasmine (default jest runner), so after discussion with @tylersmalley we have decided to exclude those paths and keep running them using jasmine2 as there are plans on migrating out of angular 🎉
https://github.com/elastic/kibana/pull/82280/files#diff-486254cfac8fc2554159849cc3bce0640c8ff69e700342b7435acfdff4ac51b7R26

The other case where I wasn't able to migrate successfully to jest-circus were integration tests. For some reason tests were not waiting for the test server to start properly, before making an assertion. If someone has an idea please let me know :)
https://github.com/elastic/kibana/pull/82671/files#diff-5d852f835ee8a9579c0b9e18fce4991f22dbfab26b2feec1beb94157b77525b8

Checklist

@patrykkopycinski patrykkopycinski added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 2, 2020
@patrykkopycinski patrykkopycinski self-assigned this Nov 2, 2020
@patrykkopycinski
Copy link
Contributor Author

angular-mocks are not compatible with jest-circus, closing it until we migrate completely from angular

@patrykkopycinski patrykkopycinski deleted the chore/jest-circus-oss branch November 4, 2020 19:36
…-modal-full-width

# Conflicts:
#	x-pack/plugins/security_solution/public/timelines/components/timeline/body/events/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/body/events/stateful_event.tsx
@patrykkopycinski patrykkopycinski restored the chore/jest-circus-oss branch November 6, 2020 14:50
@patrykkopycinski patrykkopycinski changed the title Use jest-circus in OSS Prepare OSS unit tests for jest-circus Nov 12, 2020
@patrykkopycinski patrykkopycinski marked this pull request as ready for review November 12, 2020 16:02
@patrykkopycinski patrykkopycinski requested a review from a team as a code owner November 12, 2020 16:02
@patrykkopycinski patrykkopycinski requested a review from a team November 12, 2020 16:02
@patrykkopycinski patrykkopycinski requested a review from a team as a code owner November 12, 2020 16:02
@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Straightforward to me for @elastic/kibana-presentation ...!

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks Patryk!

@tylersmalley
Copy link
Contributor

Looks like you have conflicts after #83724. Might just want to drop the version bumps as part of this PR since they were updated there.

…rcus-oss

# Conflicts:
#	packages/kbn-pm/dist/index.js
#	src/dev/jest/config.js
#	src/plugins/console/public/application/models/sense_editor/__tests__/integration.test.js
#	yarn.lock
@@ -91,7 +91,6 @@ describe('Integration', () => {
'',
function (err, terms) {
if (testToRun.assertThrows) {
done();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @patrykkopycinski ! Thanks for working on this, I did not dig into this too deeply (Console's tests are not in a good state) but I recall needing these done callbacks at one state. I am guessing these were failing before we started remove the "done" callback? Would you mind explaining the problem as you understand it (I see CI is failing on these integration tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jloleysens, so the issue was that with the new runner we cannot use both async and done in the same test, so instead of using async/await I have used .then() to make sure the assertion is run after the editor is updated and seems that solves the issue

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

const noFind = container.getChild<ContactCardEmbeddable>(embeddable.id);
expect(noFind).toBeUndefined();
container.removeEmbeddable(embeddable.id);
const promise = container.getOutput$().pipe(first()).toPromise();
Copy link
Contributor

@lizozom lizozom Dec 6, 2020

Choose a reason for hiding this comment

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

Can't we simplify these two lines to await container.getOutput$().pipe(first()).toPromise()?
I don't see anyone reusing the promise const.

(This comment is relevant to multiple places)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lizozom you were right, Thank you :)

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47141 47901 +760

History

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

@patrykkopycinski patrykkopycinski deleted the chore/jest-circus-oss branch June 10, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants