Setup full test coverage #72
Conversation
| "eslint-config-prettier": "^8.8.0", | ||
| "eslint-config-standard": "^17.0.0", | ||
| "mocha": "^10.0.0", | ||
| "mockipfs": "npm:@joewagner/mockipfs@^0.3.2", |
There was a problem hiding this comment.
I have an open PR to get these changes into the official package, but until then I published a version that works.
| export { | ||
| pinToLocal, | ||
| pinToProvider, | ||
| Pinner, |
There was a problem hiding this comment.
I didn't see an easy way to enable passing the ipfs connection details to the pinning processor, so I abstracted it into a class that get instantiated with the connection details.
This is the majority of the refactoring i ended up doing.
| Pinner, | ||
| type FileContent, | ||
| } from "./processors/pinContentToIpfs.js"; | ||
| export { default as truncate } from "./processors/truncate.js"; |
There was a problem hiding this comment.
renamed process to processors for consistency in avoiding potential collisions with the reserved word.
There was a problem hiding this comment.
Whoops, thought I already did it. Ty sir.
| const resolvedRow: RowObject = {}; | ||
| const resultsRequests = resultSet.map(async (row: RowObject) => { | ||
| // needs to be `any` since the resolver is being plugged into this package | ||
| const resolvedRow: any = {}; |
There was a problem hiding this comment.
The tests were not working with the typescript compiler because this type was [key: string]: string | number; and different processors return different types than string | number
| export class Pinner { | ||
| ipfsOptions: IPFS.Options; | ||
| ipfs: IPFS.IPFSHTTPClient; | ||
| where: string; |
There was a problem hiding this comment.
A lot of refactoring here, but I believe i preserved all the existing logic.
This is worth a solid read through.
| } | ||
|
|
||
| // TODO: why are we using the first value? | ||
| return pinningServices[0]; |
There was a problem hiding this comment.
Added a todo comment here because I'm not really sure what the implications of 2 remote services existing are. Regardless, this preserves the existing logic.
There was a problem hiding this comment.
I wasn't sure either. Perhaps we should loop through and pin to all, since a huge purpose for IPFS is to decentralize. Or pin to all by default and make any option? Don't need to change now, but worth thinking about.
| localPinner.pin.bind(localPinner), | ||
| localPinner.resolveCid.bind(localPinner) | ||
| ); | ||
| // TODO: Allow passing in the provider's endpoint. |
There was a problem hiding this comment.
This TODO was already here, I just moved it during the refactor.
| import { describe, test } from "mocha"; | ||
| import { assert } from "sinon"; | ||
| import { createProcessor, symetricEncrypt } from "../src/main"; | ||
| import fetch, { Headers, Request, Response } from "node-fetch"; |
There was a problem hiding this comment.
I removed node-fetch since the versions of node we are using for tests done need it anymore.
| Pinner, | ||
| type FileContent, | ||
| } from "./processors/pinContentToIpfs.js"; | ||
| export { default as truncate } from "./processors/truncate.js"; |
There was a problem hiding this comment.
Whoops, thought I already did it. Ty sir.
| } | ||
|
|
||
| // TODO: why are we using the first value? | ||
| return pinningServices[0]; |
There was a problem hiding this comment.
I wasn't sure either. Perhaps we should loop through and pin to all, since a huge purpose for IPFS is to decentralize. Or pin to all by default and make any option? Don't need to change now, but worth thinking about.
Fixes #70
I had to do a little bit of refactoring to enable testing the ipfs pinning, and the ipfs mock package will need to merge and publish a PR I opened before these will pass, but this gets us full coverage.