Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

testing: migration to jest #90

Merged
merged 3 commits into from
Dec 17, 2021
Merged

Conversation

przemyslaw-rzasa
Copy link
Contributor

@przemyslaw-rzasa przemyslaw-rzasa force-pushed the feature/migration-to-jest branch 4 times, most recently from 3afd7fb to 1d6bb64 Compare December 15, 2021 14:23
@przemyslaw-rzasa przemyslaw-rzasa force-pushed the feature/migration-to-jest branch 2 times, most recently from 3a442f5 to 4e88fdb Compare December 16, 2021 13:08
@@ -18,3 +18,6 @@ yarn.lock
# Mac OSX Finder files.
**/.DS_Store
.DS_Store

# Configs
!jest.config.js
Copy link
Contributor Author

@przemyslaw-rzasa przemyslaw-rzasa Dec 16, 2021

Choose a reason for hiding this comment

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

Created as js by default by ts-jest. Prompting works thanks to specific type annotation on the top of the file.

@@ -1,15 +1,18 @@
"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caused by esModuleInterop. Shouldn't have negative impact.

@@ -0,0 +1,6 @@
[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously these expectedFiles has been imported / exported directly between the test files. Right after switch to jest due to that many tests has been running multiple times and has been giving really nasty errors.

We should keep our tests as isolated as possible, to I'd suggest to always create separate file for sharable data.

@@ -65,7 +65,6 @@ export const cloudAuthenticationCognitoGenerator = (options: Schema): Rule => {
options.emails
? schematic(CloudSchematic.AUTHENTICATION_EMAILS, { name })
: noop(),
schematic("apply-prettier", {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

apply-prettier used once at the end of the main schematic should be enough I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK plan is to be able to apply some features (e.g. add authentication) to an existing application. In this case some specific schematic (not top level application schematic) will be executed, so probably it is need for those scenarios. Is that right @pieralukasz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will bring them back. Thanks for this info

@@ -18,3 +18,6 @@ yarn.lock
# Mac OSX Finder files.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest to check this file and it's dependencies firstly

*
* Useful for check, if schematics has created exactly same list of files as expected (ignores order).
*/
toHaveEqualElements(expected: Primitive[]): R;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacement for jasmine.arrayWithExactContent, but we don't have to care about the order anymore.

*
* Useful for check, if schematics has created expected files.
*/
toIncludeEvery(expected: Primitive[]): R;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally I've gave up with usage of it, as this leaves too much place for an errors. Left it, as might be suitable in the future. But can be deleted if you wish, no strong feelings here.

Copy link
Contributor

@JacekKosciesza JacekKosciesza Dec 17, 2021

Choose a reason for hiding this comment

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

Let's keep it for now, will delete it later if not used

*
* Useful for check, if schematics **does NOT** create any not expected files.
*/
toIncludeSome(expected: Primitive[]): R;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created for test cases, which proves that files from specific module haven't been created.

describe("cloud.janush", () => {
it("should generate all janush files properly", async () => {
it("should create files for: [template, janush]", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in most cases schematics are dependent on each other, maybe in a tests description we should mention about all the schematics which are in a current stack [?]

@przemyslaw-rzasa przemyslaw-rzasa force-pushed the feature/migration-to-jest branch from 4e88fdb to 365d455 Compare December 16, 2021 14:09
spyOn(janush, "readJanushJSON").and.returnValue(emptyJanush);
spyOn(janush, "updateJanushJSON");
jest.spyOn(janush, "readJanushJSON").mockReturnValue(emptyJanush);
jest.spyOn(janush, "updateJanushJSON").mockImplementation();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@przemyslaw-rzasa przemyslaw-rzasa force-pushed the feature/migration-to-jest branch 3 times, most recently from 9437465 to bb57b0d Compare December 16, 2021 16:04
@przemyslaw-rzasa
Copy link
Contributor Author

przemyslaw-rzasa commented Dec 16, 2021

Comparision

Jasmine
Screenshot 2021-12-16 at 16 34 34

Jest
Screenshot 2021-12-16 at 17 03 59

Jasmine has finished the test suites a bit earlier than jest, but we have to keep in mind that:

  • it required build command before running the tests itself to run
  • previously there was lower amount of tests set

As I've been playing with jest configuration I was able to reach jasmine score, but it required to have already built js files which was a bit pointless. Leaving it as it is for now, and if there will be any performance problems in the future, we can always come to this checkpoint 🙃

PS I've also made custom matchers as dummy as possible and removed tests for them, but it had minimal / none impact on the performance, so they seems to work ok.

@przemyslaw-rzasa przemyslaw-rzasa force-pushed the feature/migration-to-jest branch from bb57b0d to f1ee380 Compare December 16, 2021 16:11
@przemyslaw-rzasa przemyslaw-rzasa marked this pull request as ready for review December 16, 2021 16:18
@przemyslaw-rzasa przemyslaw-rzasa changed the title feature: migration to jest testing: migration to jest Dec 16, 2021
@@ -65,7 +65,6 @@ export const cloudAuthenticationCognitoGenerator = (options: Schema): Rule => {
options.emails
? schematic(CloudSchematic.AUTHENTICATION_EMAILS, { name })
: noop(),
schematic("apply-prettier", {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK plan is to be able to apply some features (e.g. add authentication) to an existing application. In this case some specific schematic (not top level application schematic) will be executed, so probably it is need for those scenarios. Is that right @pieralukasz?

*
* Useful for check, if schematics has created expected files.
*/
toIncludeEvery(expected: Primitive[]): R;
Copy link
Contributor

@JacekKosciesza JacekKosciesza Dec 17, 2021

Choose a reason for hiding this comment

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

Let's keep it for now, will delete it later if not used

@przemyslaw-rzasa przemyslaw-rzasa merged commit 722170a into main Dec 17, 2021
@przemyslaw-rzasa przemyslaw-rzasa deleted the feature/migration-to-jest branch December 17, 2021 10:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants