Add documentations for tests in json#557
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors existing Go spec tests to use the new NewControllerSpecTest (and NewMsgProcessingSpecTest) constructors, adds descriptive documentation strings to each test, and enriches the generated JSON spec files with "Type" and "Documentation" fields.
- Refactor: replace literal
&tests.ControllerSpecTest{…}and&tests.MsgProcessingSpecTest{…}with the new constructor calls - Documentation: introduce a concise description string for each Go test
- Metadata: add
"Type"and"Documentation"metadata to JSON spec test files
Reviewed Changes
Copilot reviewed 300 out of 1121 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| qbft/spectest/tests/controller/latemsg/*.go | Switched to tests.NewControllerSpecTest(...) and added test descriptions |
| qbft/spectest/tests/commit/*.go | Switched to tests.NewMsgProcessingSpecTest(...) and added test descriptions |
| qbft/spectest/generate/tests/*.json | Added "Type" and "Documentation" fields to JSON test metadata |
Comments suppressed due to low confidence (1)
qbft/spectest/generate/tests/tests.RoundRobinSpecTest_qbft_round_robin_7_member_committee.json:3
- [nitpick] Consider capitalizing 'Round Robin' in the Type field for consistency with other JSON metadata entries.
"Type": "Round robin proposer: validation of fair proposer selection algorithm",
...rate/tests/tests.MsgProcessingSpecTest_qbft_message_processing_round_change_zero_signer.json
Show resolved
Hide resolved
...sgProcessingSpecTest/qbft message processing duplicate prepare msg justification quorum.json
Outdated
Show resolved
Hide resolved
...essingSpecTest/qbft message processing duplicate rc msg justification (prepared) quorum.json
Outdated
Show resolved
Hide resolved
alan-ssvlabs
dismissed stale reviews from MatusKysel and GalRogozinski
via
ac636bd
ssv/spectest/generate/tests/committee.CommitteeSpecTest_start_with_no_shares_for_duty.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
You currently did the changes in unmaintainable manner.
If people creating parsers will complain that they prefer all types in snail case for example it will be hard to change...
More importantly go over this review please:
https://chatgpt.com/share/e/686f7fef-4c50-800a-b4ba-07d7e6e0bee8
ssv/spectest/generate/tests/committee.CommitteeSpecTest_empty_committee_duty.json
Outdated
Show resolved
Hide resolved
|
you are still missing docs to 5 tests (see chatGPT chat) |
|
Re: test names & docs for state comparison - I think what is here is great and its pretty easy to just map from the input -> state comparison to make the connection manually so its not needed for that. Thx |
There was a problem hiding this comment.
Per my latest comment there may be more issues like this.
We need to:
- Go over the test files and see if there are similar errors
- Use field name tags so we don't do such mistakes (also makes it easy to pick up in review)
- Have a dedicated string const files for types and descriptions
If possible, this would also be nice for the expected errors as I have to map from our error enums into an expected string representation |
|
@Zacholme7 |
alan-ssvlabs
dismissed stale reviews from MatusKysel and MatheusFranco99
via
597bcea
GalRogozinski
left a comment
GalRogozinski
left a comment
There was a problem hiding this comment.
Just small detail and that's it
closes #547