-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add documentation tests #147
Conversation
Documentation tests are intended for early detection of modifications on nucypher-ts that involves changes on the code examples showed in docs.
Joining all the example code blocks present in Get Started section of documentation makes the tests easier to understand, less redundant and have a more logical flow since all code shown on Get Started is part of a tutorial code and not isolated chunks of code.
test/docs/cbd.test.ts
Outdated
const MMprovider = await detectEthereumProvider(); | ||
const mumbai = providers.getNetwork(80001); | ||
|
||
if (MMprovider) { |
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 closing bracket for this if
statement seems to be misplaced: Should probably be at line 91
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.
You are right, in docs (https://docs.threshold.network/app-development/threshold-access-control-tac/get-started-with-tac#4.-build-a-strategy) the closing bracket is placed after the newStrategy.deploy() statement.
But if we place there (line 91), the variables web3Provider
and newDeployed
will not be accessible after this line, so the subsequent use of these two variables (line 95, 111, 140...) results in compilation errors.
I have been thinking about this and my best idea is to take this little trick, but other ideas are pretty welcome.
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.
My two cents:
You could create a setup
function where you run a complete scenario as shown in docs, and then return all intermediate variables. Then, to test each documentation segment, you call setup
and get any variable you need. You could move assertions into this setup
function, and leave the code examples in the test body.
That would also have the added benefit of making code examples easy to copy and paste into docs (because they only contain the relevant code).
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.
Definitely, I'm in with this idea. Working on it.
Although the closing bracket problem remains since we still need these two variables (web3Provider
and newDeployed
) after line 91 (lines 138 and 140).
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.
Another option could be to simply delete this condition from docs. I mean, delete if (MMprovider) {
line.
This is a tutorial that shows the basic usage of our code. Maybe this kind of checking is not necessary.
What do you think @piotr-roslaniec ?
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.
I think it's fine since it's a simplified (idealized) example. I.e. we're only showing a green path.
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.
My two cents:
You could create a
setup
function where you run a complete scenario as shown in docs, and then return all intermediate variables. Then, to test each documentation segment, you callsetup
and get any variable you need. You could move assertions into thissetup
function, and leave the code examples in the test body.That would also have the added benefit of making code examples easy to copy and paste into docs (because they only contain the relevant code).
Done in b5f37b2
The setup function run a a complete scenario as shown in docs. Assertions are done in test function. This makes code examples easy to copy and paste into docs.
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.
Good job 🎉
@manumonti I took the liberty of slightly refactoring this code example. Please feel free to change it as you see fit and merge it when ready
Now it looks great! Thank you |
Documentation tests are intended for early detection of modifications on nucypher-ts that involve changes on the code examples in docs.