-
Notifications
You must be signed in to change notification settings - Fork 4
Improve testing workflow #175
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
base: main
Are you sure you want to change the base?
Conversation
If we like the idea with |
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.
Very nice work @andrew-fleming, great initiative and thought on getting the testing experience smoother! Enjoyed reviewing the code.
Some points to consider:
- (IN-SCOPE) I'd argue if it is possible to have two separate workspace:
contracts/
: a workspace for all the Compact contract packages.packages/
: a workspace for all the helper TS packages (which are eventually going to be moved to another repo).
- (OUT-SCOPE) I am thinking it might also worth introducing a general class that overrides the witnesses to make it malicious for testing.
/** | ||
* Utility type that removes the context argument from circuits, | ||
* returning a version callable without the CircuitContext. | ||
*/ | ||
export type ContextlessCircuits<Circuits, TState> = { |
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.
Please let us elaborate more on the tsdocs on the generics.
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.
Done! abe5fc8
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.
General comments on the testing package:
- I think we are missing a README for this package that elaborates on how to use.
- We need to add testings here.
- Since the package is small, I would argue to to not have a sub directory
simulator
and just have all in the root rightaway. - I would have one
types.ts
script that combines allContextlessCircuits
,IContractSimulator
, and in case we agree from the suggestions we may also add bothExtractPureCircuits
andExtractImpureCircuits
. - So if we agree on point 3, and 4 the directory flow would look like this,
src/
AbstractContractSimulator.ts
types.ts
address.ts
index.ts
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.
Agreed on all points
Since the package is small, I would argue to to not have a sub directory simulator and just have all in the root rightaway.
I wasn't sure how big this was going to be, but yeah. For now, I think flat makes sense 👍
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.
Points 3, 4, 5 ✅
Tests and readme are forthcoming
"name": "@openzeppelin-compact/testing", | ||
"private": true, | ||
"version": "0.0.1", | ||
"description": "", |
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.
Please let us add description here.
packages/contracts/nonFungibleToken/src/test/simulators/NonFungibleTokenSimulator.ts
Outdated
Show resolved
Hide resolved
packages/contracts/nonFungibleToken/src/test/simulators/NonFungibleTokenSimulator.ts
Outdated
Show resolved
Hide resolved
packages/contracts/nonFungibleToken/src/test/simulators/NonFungibleTokenSimulator.ts
Outdated
Show resolved
Hide resolved
I like the idea, I would vote for yes. But I am thinking that could be anther class to be abstracted and the implementing Simulator have the option to extend/implement it. |
Thank you for the review @0xisk!
I initially had it set up this way^. I thought it looked weird with a
100%! Life will be much easier :) |
Co-authored-by: 0xisk <[email protected]> Signed-off-by: Andrew Fleming <[email protected]>
Co-authored-by: 0xisk <[email protected]> Signed-off-by: Andrew Fleming <[email protected]>
The following PR proposes to improve the testing workflow with the following changes:
1. Create a
testing/
workspacepure
andimpure
circuits using theContextlessCircuits
typeContextlessCircuits
type removes the ctx arg from circuitsSimulator example
This allows us to change this:
into this:
Test example
In the actual tests, we can dynamically set the caller context and remove the extra
caller
arg.So again we can change this:
into this:
Address utils
A little less sexy: All contract tests can import from one source for testing utils such as
toHexPadded
,encodeToPk
, etcBUT the really really good part of this is that it allows the Foo
test/
directory to change from:to this:
2. Add compact-std
This removes the need to export compact types from mocks (credit to @0xisk 🙌 )
3. Move all packages to
packages/
contracts/
compact/
compact-std/
testing/
It looks super unorganized having all these different directories sitting in root. This improves organization
Finally, this PR only proposes to apply the testing changes to one module (
nonFungibleToken
) as a proof of concept. This reduces the PR noise to make it easier to review. This allows any discussions to focus mostly on design instead of getting lost in 12498129423498 file diffs. We can open a new PR applying the changes to the rest if/when this is merged (or in this PR if preferred)If this is too much, happy to break this up into separate PRs
Fixes #32
Fixes #58
Fixes #59
Fixes #74
PR Checklist