Add unit tests in Clarity #4100

friedger · 2023-11-29T11:15:27Z

Description

This PR adds the ability to write clarinet-sdk tests in Clarity.

As an example a simple unit tests and a simple flow tests was added. These tests do not test anything meaningful, but show how these tests work.

Applicable issues

fixes [clarity] Setup Standard Clarinet Project For PoX-4 Flow Tests #3976

Additional info (benefits, drawbacks, caveats)

Checklist

Test coverage for new or modified code paths
Changelog is updated
Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

codecov · 2023-11-30T17:56:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8cffc38) 85.08% compared to head (fedc750) 82.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #4100      +/-   ##
==========================================
- Coverage   85.08%   82.06%   -3.03%     
==========================================
  Files         429      429              
  Lines      302009   302009              
==========================================
- Hits       256976   247832    -9144     
- Misses      45033    54177    +9144

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

moodmosaic

LGTM

moodmosaic · 2023-12-03T13:34:32Z

contrib/core-contract-tests/contracts/bns-tests/bns_flow_test.clar

+(define-constant ERR_NAMESPACE_NOT_FOUND 1005)
+
+;; @name: test delegation to wallet_2, stacking and revoking
+(define-public (test-name-registration)


In a future PR I think it's doable (and a bit challenging too) to enable tests with parameters, to run those tests also as fuzz tests:

(define-public (test-name-registration (hashed-salted-fqn (buff 20)) (stx-to-burn uint) (namespace (buff 20)) (name (buff 48)) (salt (buff 20)) (zonefile-hash (buff 20)))

The tricky part might be the extra work around regexes, though the heavily lifting part around fast-check shouldn't be an issue.

I think we can specify parameter testing / fuzzing with annotations. The regexes originally came from a concept implementation to enable us to write Clarity tests in Clarity. After that, they were good enough so we just left it. If we want to make it more robust we will need to switch to a LISP interpreter, should not be too hard.

If we can sort this part out, I can integrate this with fast-check and we'll have it generate/fuzz everything for us. What's very nice about this is the shrinking part; if there's a bug/edge-case detected, fast-check will give us the minimum counterexample and a seed to reproduce the failure (if the bug/edge-case is detected when running on CI).

MarvinJanssen

Naturally I much prefer writing tests in Clarity. The question is do the other code owners like (or at least are they OK with) bringing this new testing paradigm into the repository. Who decides?

MarvinJanssen · 2023-12-04T16:32:38Z

contrib/core-contract-tests/contracts/bns-tests/bns_flow_test.clar

+(define-constant ERR_NAMESPACE_NOT_FOUND 1005)
+
+;; @name: test delegation to wallet_2, stacking and revoking
+(define-public (test-name-registration)


I think we can specify parameter testing / fuzzing with annotations. The regexes originally came from a concept implementation to enable us to write Clarity tests in Clarity. After that, they were good enough so we just left it. If we want to make it more robust we will need to switch to a LISP interpreter, should not be too hard.

jcnelson · 2023-12-05T03:54:46Z

contrib/core-contract-tests/tests/clar/clar-flow.test.ts

@@ -0,0 +1,138 @@
+import { ParsedTransactionResult, tx } from "@hirosystems/clarinet-sdk";


Hey, do you think you can add some method-level comments for each of these functions in this file and the clar.test.ts file? I'm having a hard time following how these functions fit together. Thanks!

This parser evolved over time during our work on sBTC Mini. We just discussed in the Clarity WG to clean it up and perhaps introduce a more robust parser. The current files have served our needs and were recently edited to support clarinet-sdk.

Comments have been added

jcnelson · 2023-12-05T03:56:20Z

contrib/core-contract-tests/tests/clar/utils/clarity-parser.ts

+
+export function getContractName(contractId: string) {
+  return contractId.split(".")[1];
+}


This parser looks somewhat involved. Is there a way to add a (simple) set of unit tests to verify that all the test parsers work as intended? Thanks!

@friedger @jcnelson happy to do this with fast-check? This way we’ll test and have the ability to also fuzz the interpreter at the same time.

Code improvements have been pushed

FYI, some prop/fuzz tests I added can be found in 3e9b3af and 3420904.

…sed tests - Implemented property-based testing for string, uint, and tuple conversions using fast-check. - Defined custom arbitraries for generating 128-bit unsigned (uint128) and signed (int128) integers. - Enhanced test robustness and coverage by automatically testing a wide range of input values. - Included fast-check's shrinking feature to identify minimal failure cases, aiding in effective debugging.

setzeus

Pulled & followed the logic due to familiarity w/ mini. My only feedback here is to make an issue for documenting how these tests are built & how a contributor can write additional tests.

LGTM.

wileyj · 2023-12-18T16:12:34Z

@MarvinJanssen @friedger I like the approach here, and what you're trying to do.
however, i think it might be better to think about these changes as something that would be easier to maintain as a separate repo entirely (possibly even a monorepo with a few other testing repos that i know of).

my concern is that if we need to make changes, the bar to getting them merged to master is much higher in this repo vs a separate repo.

merging to next, and updating next is less of an issue i'll agree. i'm thinking longer-term though, when eventually next is merged to master and we may find issues/improvement that should be made. having this in a separate repo would make those changes much easier to make.

friedger · 2023-12-18T21:03:31Z

@wileyj I agree.

As there is currently no other repo, this seems to be the only solution to get a good discussion.

Merging a version update of the tool into main should be as hard as updating the source code 😉

wileyj · 2023-12-19T06:45:23Z

@wileyj I agree.

As there is currently no other repo, this seems to be the only solution to get a good discussion.

Merging a version update of the tool into main should be as hard as updating the source code 😉

that's what concerns me actually - unless i'm misunderstanding the reasoning behind this PR, i see a lot of benefit in moving most (or all) of the tooling outside of the blockchain repo (we can still maintain a higher bar for merging PR's, but maybe not quite as high as the blockchain repo requires).
the tests themselves, i have no opinion on - i'm mostly looking at the tooling code/dirs in this PR.
we can discuss wednesday, but the goal for me is to make updating the tooling as easy as it needs to be.

obycode · 2023-12-19T15:33:18Z

Being discussed tomorrow.

friedger · 2024-01-10T09:09:12Z

using clarunit now: #4203 (comment)

feat: add unit tests in clarity

3757a97

friedger mentioned this pull request Nov 29, 2023

[clarity] Setup Standard Clarinet Project For PoX-4 Flow Tests #3976

Closed

friedger changed the base branch from master to next November 29, 2023 12:58

chore: add log file for flow tests

6f3f983

chore: push package-lock.json

7abb389

friedger requested a review from hugocaillard December 1, 2023 20:18

friedger marked this pull request as ready for review December 1, 2023 20:19

friedger requested review from setzeus and MarvinJanssen December 1, 2023 20:19

moodmosaic reviewed Dec 3, 2023

View reviewed changes

MarvinJanssen approved these changes Dec 4, 2023

View reviewed changes

jcnelson reviewed Dec 5, 2023

View reviewed changes

friedger and others added 7 commits December 5, 2023 16:15

chore: add comments

f77f32e

chore: improve code readability

c5f9e4a

chore: add more tests

748a330

Merge branch 'next' into feat/clarinet-flow-tests

c85906d

feat(tests): Add clarity-parser.prop.test.ts property-based tests

3420904

Merge branch 'next' into feat/clarinet-flow-tests

1755973

setzeus approved these changes Dec 12, 2023

View reviewed changes

Merge branch 'next' into feat/clarinet-flow-tests

35d51ea

setzeus mentioned this pull request Dec 16, 2023

pox-4: disallow stacking during prepare phase #4094

Closed

2 tasks

MarvinJanssen approved these changes Dec 18, 2023

View reviewed changes

Merge branch 'next' into feat/clarinet-flow-tests

fedc750

setzeus mentioned this pull request Dec 20, 2023

Creating 'stacks-testing-tools' Repo #4203

Closed

setzeus mentioned this pull request Jan 4, 2024

PoX-4 Clarunit Setup & Initial Tests #4231

Merged

friedger closed this Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests in Clarity #4100

Add unit tests in Clarity #4100

friedger commented Nov 29, 2023

codecov bot commented Nov 30, 2023 •

edited

Loading

moodmosaic left a comment

moodmosaic Dec 3, 2023

MarvinJanssen Dec 4, 2023

moodmosaic Dec 4, 2023

MarvinJanssen left a comment

MarvinJanssen Dec 4, 2023

jcnelson Dec 5, 2023

MarvinJanssen Dec 5, 2023

friedger Dec 12, 2023

jcnelson Dec 5, 2023

moodmosaic Dec 5, 2023

friedger Dec 12, 2023

moodmosaic Dec 19, 2023

setzeus left a comment

wileyj commented Dec 18, 2023

friedger commented Dec 18, 2023

wileyj commented Dec 19, 2023

obycode commented Dec 19, 2023

friedger commented Jan 10, 2024

		@@ -0,0 +1,138 @@
		import { ParsedTransactionResult, tx } from "@hirosystems/clarinet-sdk";

Add unit tests in Clarity #4100

Add unit tests in Clarity #4100

Conversation

friedger commented Nov 29, 2023

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

codecov bot commented Nov 30, 2023 • edited Loading

Codecov Report

moodmosaic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarvinJanssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setzeus left a comment

Choose a reason for hiding this comment

wileyj commented Dec 18, 2023

friedger commented Dec 18, 2023

wileyj commented Dec 19, 2023

obycode commented Dec 19, 2023

friedger commented Jan 10, 2024

codecov bot commented Nov 30, 2023 •

edited

Loading