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

Complete the Jest DI with crypto #234

Merged
merged 26 commits into from
May 27, 2021
Merged

Complete the Jest DI with crypto #234

merged 26 commits into from
May 27, 2021

Conversation

matheus23
Copy link
Contributor

This is preparation work for making it possible to write more sophisticated test, making test-driven-development for internal (non-browser-bundle-exposed) APIs possible.

src/common/version.ts Outdated Show resolved Hide resolved
Comment on lines 119 to 128
async getSymmKey(keyName: string, cfg?: Partial<Config>): Promise<CryptoKey> {
const mergedCfg = config.merge(this.cfg, cfg)
const maybeKey = this.store[keyName]
if(maybeKey !== null) {
return maybeKey
}
const key = await aes.makeKey(config.symmKeyOpts(mergedCfg))
this.store[keyName] = key
return key
}
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 duplicate some code here from keystore-idb.

We should probably eventually make it possible to use our dependencies in node. However, doing that for keystore-idb could be questionable, given it has "idb" in its name 😄

Any way, this feels kind of quick-and-dirty right now. Let me know if you have any other ideas.

matheus23 added 2 commits May 18, 2021 22:38
Switch to using Uint8Arrays instead of node Buffers.
This is something the js-ipfs folks apparently have gone through, too:
ipfs/js-ipfs#3220
@matheus23 matheus23 marked this pull request as draft May 19, 2021 16:54
@matheus23 matheus23 marked this pull request as ready for review May 19, 2021 21:35
Copy link
Contributor

@walkah walkah left a comment

Choose a reason for hiding this comment

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

This is great! thanks for your continued efforts getting this working!

I Appreciate You - KD22dF8apIKLSapKSC

FYI: bcrypto isn't currently building for me locally in the nix shell on macOS. I'm going to poke around, but it seems to work in CI so may be my environment.

@@ -36,7 +36,7 @@
"prebuild": "rimraf dist && node scripts/gen-version.js",
"build": "tsc && rollup -c rollup.config.ts",
"start": "tsc -w",
"test": "jest",
"test": "jest --forceExit",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. This smells like there's something wrong with the tests... async operations not resolving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I tried to track it down for a while, but gave up. From my testing it seems like in some edge-cases it's possible that ipfs leaves a child process and some sockets hanging after await ipfs.stop(). Not sure what's up with that, but after fighting with it for a while I just gave up.
The IPFS we're using in the tests is basically in-memory only. It doesn't write to disk. So killing it shouldn't be an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. yeah, something we can track down and fix as a follow up later, imo.

@@ -1,8 +1,9 @@
import crypto from "crypto"
import Ipfs, { IPFS } from "ipfs"
import { IPFS } from "ipfs-core"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a size thing? (full IPFS includes more code than we use, etc?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly. It's just much less dependencies.
I think the difference is that ipfs also contains the CLI and an http-server, which ipfs-core does not.

@matheus23
Copy link
Contributor Author

FYI: bcrypto isn't currently building for me locally in the nix shell on macOS. I'm going to poke around, but it seems to work in CI so may be my environment.

Yeah. Weird. bcrypto is building fine on my end (including in the nix shell).

async function runRoundTrip(message) {
const keyStr = await webnative.crypto.aes.genKeyStr()

const encoded = webnative.cbor.encode(message)
Copy link
Member

Choose a reason for hiding this comment

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

This test is failing for me with:

Evaluation failed: TypeError: Cannot read property 'encode' of undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you yarn build before you yarn tested?

Copy link
Member

Choose a reason for hiding this comment

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

I did not 🤦 and that was it. After building, this test and all the others are passing for me 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Admittedly, it's really confusing. The puppeteer html file fixture uses the index.umd.min.js file, so unless one yarn builds before running the tests, it'll use an outdated webnative, but only for the puppeteer stuff.

Copy link
Member

@bgins bgins left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for implementing these Jest DI crypto functions 🎉 🙏

@matheus23 matheus23 merged commit ef3f1c9 into main May 27, 2021
@matheus23 matheus23 deleted the matheus23/complete-jest-di branch May 27, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants