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

fix(jest-config): accept snapshotFormat.compareKeys option #12387

Closed

Conversation

Andarist
Copy link
Contributor

The configuration docs states that:

snapshotFormat [object]
[...]
Allows overriding specific snapshot formatting options documented in the pretty-format readme. For example, this config would have the snapshot formatter not print a prefix for "Object" and "Array":

However, I've found out that it's not possible to configure snapshotFormat.compareKeys from within jest.config.js file.

The reason behind this is that you are outsourcing correct types for options to the DEFAULT_OPTIONS from the pretty-format that are declared here:
https://github.com/facebook/jest/blob/c8a84566be774a35a56c26a4595c553df5fc6fc1/packages/pretty-format/src/index.ts#L401
and this is used during validation. But the correct type for snapshotFormat.compareKeys doesn't match the typeof DEFAULT_OPTIONS.compareKeys.

Comment on lines +114 to +117
snapshotFormat: {
...PRETTY_FORMAT_DEFAULTS,
compareKeys: () => {},
} as SnapshotFormat,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I proud of this fix? No.
Does it work? Yes.

I'm afraid that I won't have time to work on the improved fix though - this is somewhat problematic because this thing is handled through a very generic validation function and this case here is very specific.

If you have any suggestions on the improved design that could be implemented quickly I could take a stab at this but I'm worried that an improved architecture around this might not be a change for a couple of lines of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably the long-run plan is to use recently added schemas for validation purposes? This would IMHO go outside of the scope of this PR as it's a bigger refactor.

Copy link
Member

Choose a reason for hiding this comment

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

Probably the long-run plan is to use recently added schemas for validation purposes?

Correct 🙂

{
ci: false,
rootDir: '/root/',
snapshotFormat: {compareKeys} as SnapshotFormat,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those casts could be removed with such a patch:

diff --git a/packages/jest-config/src/ValidConfig.ts b/packages/jest-config/src/ValidConfig.ts
index db7ea5fe6d..1a4348d5dc 100644
--- a/packages/jest-config/src/ValidConfig.ts
+++ b/packages/jest-config/src/ValidConfig.ts
@@ -5,7 +5,6 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import type {SnapshotFormat} from '@jest/schemas';
 import type {Config} from '@jest/types';
 import {replacePathSepForRegex} from 'jest-regex-util';
 import {multipleValidOptions} from 'jest-validate';
@@ -113,8 +112,8 @@ const initialOptions: Config.InitialOptions = {
   slowTestThreshold: 5,
   snapshotFormat: {
     ...PRETTY_FORMAT_DEFAULTS,
-    compareKeys: () => {},
-  } as SnapshotFormat,
+    compareKeys: () => 0,
+  },
   snapshotResolver: '<rootDir>/snapshotResolver.js',
   snapshotSerializers: ['my-serializer-module'],
   testEnvironment: 'jest-environment-node',
diff --git a/packages/jest-config/src/__tests__/normalize.test.ts b/packages/jest-config/src/__tests__/normalize.test.ts
index fb229b4e33..7b858656ee 100644
--- a/packages/jest-config/src/__tests__/normalize.test.ts
+++ b/packages/jest-config/src/__tests__/normalize.test.ts
@@ -9,7 +9,6 @@
 import {createHash} from 'crypto';
 import path from 'path';
 import semver = require('semver');
-import type {SnapshotFormat} from '@jest/schemas';
 import type {Config} from '@jest/types';
 import {escapeStrForRegex} from 'jest-regex-util';
 import Defaults from '../Defaults';
@@ -1928,11 +1927,11 @@ describe('snapshotFormat', () => {
       {
         ci: false,
         rootDir: '/root/',
-        snapshotFormat: {compareKeys} as SnapshotFormat,
+        snapshotFormat: {compareKeys},
       },
       {} as Config.Argv,
     );
 
-    expect((options.snapshotFormat as any).compareKeys).toBe(compareKeys);
+    expect(options.snapshotFormat.compareKeys).toBe(compareKeys);
   });
 });
diff --git a/packages/jest-schemas/src/index.ts b/packages/jest-schemas/src/index.ts
index b3f0a32eeb..09a31cff5b 100644
--- a/packages/jest-schemas/src/index.ts
+++ b/packages/jest-schemas/src/index.ts
@@ -10,6 +10,9 @@ import {Static, Type} from '@sinclair/typebox';
 const RawSnapshotFormat = Type.Partial(
   Type.Object({
     callToJSON: Type.Readonly(Type.Boolean()),
+    compareKeys: Type.Readonly(
+      Type.Function([Type.String(), Type.String()], Type.Number()),
+    ),
     escapeRegex: Type.Readonly(Type.Boolean()),
     escapeString: Type.Readonly(Type.Boolean()),
     highlight: Type.Readonly(Type.Boolean()),

But I'm not sure if I should apply it because it feels like maybe compareKeys was omitted intentionally when declaring schemas in #12384

@SimenB
Copy link
Member

SimenB commented Feb 14, 2022

This won't actually works as we serialize the options so we can pass them to workers/child processes. So functions won't survive. (it should work when running in band, but not in other cases).

If we are to support compareKeys (and plugins) it need to point to a file that exports a function.


We should update the docs in the meantime so they reflect reality

@Andarist
Copy link
Contributor Author

Oh, that makes total sense and might explain some weirdness that I've experienced 🙈

In fact... I don't even need this because my use case is simpler. I'd just like to disable the sorting of the property keys. It's important for me to verify the order of the keys because we are creating snapshots of the package.json#exports and the order within it is very important (those conditions/paths etc are matched from the top to the bottom). Would you perhaps be open to introducing a new option to the pretty-format that would just disable sorting? This way I could configure it with a simple, serializable, boolean.

A sideways note is that... while I've initially attempted to configure this the whole thing has crashed in a not very DX-friendly way. undefined got its way as value here:
https://github.com/facebook/jest/blob/c8a84566be774a35a56c26a4595c553df5fc6fc1/packages/jest-validate/src/utils.ts#L25
and crashed the runtime.

@SimenB
Copy link
Member

SimenB commented Feb 14, 2022

Pushed 6baf127 correcting the docs.


Would you perhaps be open to introducing a new option to the pretty-format that would just disable sorting?

Sure. I think I'd prefer overloading the existent function parameter, tho. E.g. supporting some magic string (alphabetical, none) from config. Then pretty-format itself can in addition support a function.

@codecov-commenter
Copy link

Codecov Report

Merging #12387 (6baf127) into main (c8a8456) will not change coverage.
The diff coverage is n/a.

❗ Current head 6baf127 differs from pull request most recent head 47df142. Consider uploading reports for the commit 47df142 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #12387   +/-   ##
=======================================
  Coverage   68.48%   68.48%           
=======================================
  Files         325      325           
  Lines       16971    16971           
  Branches     5061     5061           
=======================================
  Hits        11622    11622           
  Misses       5317     5317           
  Partials       32       32           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8a8456...47df142. Read the comment docs.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants