Skip to content

Generate stable JSON from generate_spec_json.js #2843

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 3, 2020
Merged

Generate stable JSON from generate_spec_json.js #2843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bbeee9
Generate stable JSON from generate_spec_json.js
dconeybe Mar 31, 2020
5ba70ef
[AUTOMATED]: License Headers
dconeybe Mar 31, 2020
aafc981
Address code review feedback, round 1.
dconeybe Mar 31, 2020
6696071
Pin versions of json-stable-stringify in package.json to a specific v…
dconeybe Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/firestore/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
"@firebase/app-types": "0.x"
},
"devDependencies": {
"@types/json-stable-stringify": "1.0.32",
"json-stable-stringify": "1.0.1",
"protobufjs": "6.8.9",
"rollup": "2.0.6",
"rollup-plugin-copy-assets": "1.1.0",
Expand Down
65 changes: 63 additions & 2 deletions packages/firestore/test/unit/specs/describe_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -19,11 +19,14 @@ import { ExclusiveTestFunction, PendingTestFunction } from 'mocha';

import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence';
import { assert } from '../../../src/util/assert';
import { primitiveComparator } from '../../../src/util/misc';
import { addEqualityMatcher } from '../../util/equality_matcher';

import { SpecBuilder } from './spec_builder';
import { SpecStep } from './spec_test_runner';

import * as stringify from 'json-stable-stringify';
Copy link
Contributor

Choose a reason for hiding this comment

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

It's less important in tests but we should be getting in the habit of avoiding whole-module imports like this.

However, I'm not actually sure what the right way to do this import is. Does import stringify from 'json-stable-stringify' work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, no, not with our current tsconfig.json. I get the following error:

index.d.ts(25, 1): This module is declared with using 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.

That being said, I'm not very familiar with TypeScript imports and all of the other variants that I tried did not work. Any other suggestions that I could try? In case it helps, here is what the index.d.ts looks like: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/json-stable-stringify/index.d.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it looks like we're missing the esModuleInterop compiler setting.

@Feiyang1 is there any reason not to enable this repo-wide? We're already using it in our integration tests.

Copy link
Member

@Feiyang1 Feiyang1 Apr 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We can enable it. I'm actually surprised to see that Typescript allows you to use stringify as a function directly. Thought it will give an error as it's not compliant with es6 modules spec.

Copy link
Member

Choose a reason for hiding this comment

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

#2849


// Disables all other tests; useful for debugging. Multiple tests can have
// this tag and they'll all be run (but all others won't).
const EXCLUSIVE_TAG = 'exclusive';
Expand Down Expand Up @@ -242,7 +245,65 @@ export function describeSpec(
} else {
specsInThisTest = {};
builder();
const output = JSON.stringify(specsInThisTest, null, 2);
// Note: We use json-stable-stringify instead of JSON.stringify() to ensure
// that the generated JSON does not produce diffs merely due to the order
// of the keys in an object changing.
const output = stringify(specsInThisTest, {
space: 2,
cmp: stringifyComparator
});
writeJSONFile(output);
}
}

/**
* The key ordering overrides used when sorting keys in the generated JSON.
* If both keys being compared are present in this array then they should be
* ordered in the generated JSON in the same relative order of this array.
*/
const stringifyCustomOrdering = [
'comment',
'describeName',
'itName',
'tags',
'config',
'steps'
];

/**
* Assigns a "group number" to the given key in the generated JSON.
*
* Keys with lower group numbers should be ordered before keys with greater
* group numbers. Keys with equal group numbers should be ordered
* alphabetically.
*/
function stringifyGroup(s: string): number {
const index = stringifyCustomOrdering.indexOf(s);
if (index >= 0) {
return index;
} else if (!s.startsWith('expected')) {
return stringifyCustomOrdering.length;
} else {
return stringifyCustomOrdering.length + 1;
}
}

/**
* A comparator function for stringify() that sorts keys in the JSON output.
*
* In order to produce JSON that has somewhat-intuitively-ordered keys, this
* comparator intentionally deviates from pure alphabetic ordering, placing
* some logically-first keys before others.
*/
function stringifyComparator(
a: stringify.Element,
b: stringify.Element
): number {
const aGroup = stringifyGroup(a.key);
const bGroup = stringifyGroup(b.key);
if (aGroup === bGroup) {
return primitiveComparator(a.key, b.key);
} else {
return primitiveComparator(aGroup, bGroup);
}
}
12 changes: 12 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2116,6 +2116,11 @@
resolved "https://registry.npmjs.org/@types/json-schema/-/json-schema-7.0.3.tgz#bdfd69d61e464dcc81b25159c270d75a73c1a636"
integrity sha512-Il2DtDVRGDcqjDtE+rF8iqg1CArehSK84HZJCT7AMITlyXRBpuPhqGLDQMowraqqu1coEaimg4ZOqggt6L6L+A==

"@types/[email protected]":
version "1.0.32"
resolved "https://registry.npmjs.org/@types/json-stable-stringify/-/json-stable-stringify-1.0.32.tgz#121f6917c4389db3923640b2e68de5fa64dda88e"
integrity sha512-q9Q6+eUEGwQkv4Sbst3J4PNgDOvpuVuKj79Hl/qnmBMEIPzB5QoFRUtjcgcg2xNUZyYUGXBk5wYIBKHt0A+Mxw==

"@types/long@*", "@types/long@^4.0.0":
version "4.0.0"
resolved "https://registry.npmjs.org/@types/long/-/long-4.0.0.tgz#719551d2352d301ac8b81db732acb6bdc28dbdef"
Expand Down Expand Up @@ -8919,6 +8924,13 @@ json-stable-stringify-without-jsonify@^1.0.1:
resolved "https://registry.npmjs.org/json-stable-stringify-without-jsonify/-/json-stable-stringify-without-jsonify-1.0.1.tgz#9db7b59496ad3f3cfef30a75142d2d930ad72651"
integrity sha1-nbe1lJatPzz+8wp1FC0tkwrXJlE=

[email protected]:
version "1.0.1"
resolved "https://registry.npmjs.org/json-stable-stringify/-/json-stable-stringify-1.0.1.tgz#9a759d39c5f2ff503fd5300646ed445f88c4f9af"
integrity sha1-mnWdOcXy/1A/1TAGRu1EX4jE+a8=
dependencies:
jsonify "~0.0.0"

json-stable-stringify@~0.0.0:
version "0.0.1"
resolved "https://registry.npmjs.org/json-stable-stringify/-/json-stable-stringify-0.0.1.tgz#611c23e814db375527df851193db59dd2af27f45"
Expand Down