Conversation
product-security-plaid-production
bot
requested review from
fernandofederico1984 and
ryannelsonsmartcontract
|
👋 nolag, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ing or encoding Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
nolag
force-pushed
the
rtinianov_testFramework
branch
from
1c3c28e to
b8d6ae7
Compare
ernest-nowacki
left a comment
ernest-nowacki
left a comment
There was a problem hiding this comment.
This is great! Docs are well structured and examples easy to follow. I would probably change the relative paths so the generated code start from @cre .
Generally it's mostly nitpicking. I'll write some unit tests myself this week (I'm thinking coverage for workflows I have under packages/cre-sdk-examples) and can always follow up with potential improvements if I have fresh ideas 👌
There was a problem hiding this comment.
Docs for this are great 👏
| "test": "bun test", | ||
| "test:standard": "./scripts/run-standard-tests.sh", | ||
| "typecheck": "tsc" | ||
| "typecheck": "tsc -p tsconfig.json --noEmit" |
There was a problem hiding this comment.
Why was this needed? tsconfig.json should be picked by default if not specified otherwise and --noEmit shouldn't be needed either as it's part of the tsconfig.json itself.
| // Use JSON.stringify to correctly escape backslashes, quotes, and control characters, | ||
| // then strip the surrounding quotes it adds. |
There was a problem hiding this comment.
Props for comment 👍
| const dirDepth = dirname(file.name).split('/').length + 2 | ||
| const registerImportPath = '../'.repeat(dirDepth) + 'testutils/test-runtime' |
There was a problem hiding this comment.
What potentially could work is just const registerImportPath = '@cre/sdk/testutils/test-runtime' (non issue, just readability and non-blocking).
| const capIdParts = capOption.capabilityId.split('@') | ||
| const capabilityName = capIdParts[0] | ||
| const capabilityVersion = capIdParts[1] || '' |
There was a problem hiding this comment.
Another tiny thing is that we could have reusable way of extracting capability names and versions shared in generator/utils so both regular generator and mock generator can have it the same way.
export function parseCapabilityId(capabilityId: string): { name: string; version: string } {
const [name, version = ''] = capabilityId.split('@')
return { name, version }
}
| __getTestMockInstance, | ||
| __setTestMockInstance, | ||
| registerTestCapability, | ||
| } from '../../../../../../testutils/test-runtime' |
There was a problem hiding this comment.
This is connected to the way we resolve paths in generator - as you can see above other generated code just uses @cre/generated/... instead of relative paths, which I find easier to follow.
3 participants
Draft PR showing how the templates will be updated to use this. Note, I used a local override for the SDK. If you want to try it locally, you'll need to update package.json to expose the file reference and update the file location.
Next PRs