-
Notifications
You must be signed in to change notification settings - Fork 906
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
Copy over fast-stable-stringify
tests from source
#2501
Copy over fast-stable-stringify
tests from source
#2501
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @steveluscher and the rest of your teammates on Graphite |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
f1b2a60
to
4631791
Compare
"test:unit:browser": "jest -c ../../node_modules/@solana/test-config/jest-unit.config.browser.ts --globalSetup @solana/test-config/test-validator-setup.js --globalTeardown @solana/test-config/test-validator-teardown.js --rootDir . --silent", | ||
"test:unit:node": "jest -c ../../node_modules/@solana/test-config/jest-unit.config.node.ts --globalSetup @solana/test-config/test-validator-setup.js --globalTeardown @solana/test-config/test-validator-teardown.js --rootDir . --silent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never needed the test validator.
4631791
to
013e9b3
Compare
], | ||
], | ||
])('matches the output of `json-stable-stringify` when hashing %s (`%s`)', (_, value) => { | ||
expect(stringify(value)).toBe(jsonStableStringify(value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to bench this against anything else, like faster-stable-stringify
or JSON.stringify
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original fast-stable-stringify
tests verified its output against json-stable-stringify
for correctness only.
I at least wanted to use that as a starting point so that as we make changes we can observe how they make ours diverge.
"test:unit:node": "jest -c ../../node_modules/@solana/test-config/jest-unit.config.node.ts --rootDir . --silent" | ||
}, | ||
"devDependencies": { | ||
"@types/json-stable-stringify": "^1.0.36", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why it needs the types, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json-stable-stringify
doesn't use TypeScript. You have to buy the types on the second hand market.
Merge activity
|
🎉 This PR is included in version 1.91.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
No description provided.