Skip to content

docs(ctb): Document the test naming convention#4116

Closed
maurelian wants to merge 5 commits intodevelopfrom
jm/ctb-test-cleanup/base
Closed

docs(ctb): Document the test naming convention#4116
maurelian wants to merge 5 commits intodevelopfrom
jm/ctb-test-cleanup/base

Conversation

@maurelian
Copy link
Contributor

@maurelian maurelian commented Nov 30, 2022

Description

Furthers the work of reorganizing the forge tests, which was begun in #3611.
This is a smaller version of #4084.

This PR does not move or reorganize functions, it only renames them.

It also:

  1. Adds documentation for the naming convention to the README.
  2. Adds a ts script which checks to ensure that tests conform to the convention. It can be run from within the contracts-bedrock package using ts-node scripts/forges-test-names.ts.

This script should eventually be added to CI, once all function names have been updated.

Longer term the rules in the script should be added to a more robust set of internal rules, defined in slither or a similar tool.

@changeset-bot
Copy link

changeset-bot bot commented Nov 30, 2022

🦋 Changeset detected

Latest commit: e5eed49

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@eth-optimism/contracts-bedrock Patch
@eth-optimism/actor-tests Patch
@eth-optimism/sdk Patch
@eth-optimism/drippie-mon Patch
@eth-optimism/message-relayer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@maurelian maurelian force-pushed the jm/ctb-test-cleanup/base branch from 0fff8e5 to 66a19f5 Compare November 30, 2022 21:11
@maurelian maurelian changed the title docs(ctb): Document the testing conventions docs(ctb): Document the test naming convention Nov 30, 2022
@maurelian maurelian force-pushed the jm/ctb-test-cleanup/base branch from 66a19f5 to 20ca4d6 Compare November 30, 2022 21:16

contract AddressAliasHelper_Test is Test {
function test_fuzz_roundtrip(address _address) external {
function testFuzz_applyAndUndo_succeeds(address _address) external {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I'm now leaning towards removing the _succeeds postfix, but I'd like to get these changes in as is, then we could easily cut it out in a subsequent PR.

@maurelian maurelian marked this pull request as ready for review November 30, 2022 21:23
@maurelian maurelian requested a review from clabby November 30, 2022 21:23
@maurelian maurelian force-pushed the jm/ctb-test-cleanup/base branch from 20ca4d6 to e5eed49 Compare November 30, 2022 21:28
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2022

Codecov Report

Merging #4116 (e5eed49) into develop (87aa40d) will increase coverage by 2.51%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4116      +/-   ##
===========================================
+ Coverage    39.51%   42.02%   +2.51%     
===========================================
  Files          287      295       +8     
  Lines        16231    16515     +284     
  Branches       642      641       -1     
===========================================
+ Hits          6414     6941     +527     
+ Misses        9307     9063     -244     
- Partials       510      511       +1     
Flag Coverage Δ
bedrock-go-tests 37.62% <ø> (-0.04%) ⬇️
contracts-bedrock-tests 38.40% <ø> (ø)
contracts-governance-tests ?
contracts-periphery-tests 95.17% <ø> (ø)
contracts-tests 98.86% <ø> (?)
core-utils-tests ?
dtl-tests 47.15% <ø> (ø)
fault-detector-tests 34.18% <ø> (ø)
sdk-tests 39.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
op-node/p2p/discovery.go 65.97% <0.00%> (-2.41%) ⬇️
packages/core-utils/src/common/bn.ts
.../contracts-governance/contracts/test/TestERC20.sol
packages/core-utils/src/optimism/encoding.ts
packages/core-utils/src/optimism/op-provider.ts
packages/core-utils/src/common/index.ts
packages/core-utils/src/common/hex-strings.ts
packages/core-utils/src/external/geth/index.ts
...ore-utils/src/external/ethers/fallback-provider.ts
packages/core-utils/src/optimism/hashing.ts
... and 50 more

@smartcontracts
Copy link
Contributor

I feel like this should be multiple PRs. Doc change itself could be one PR, then PR to add the script, then multiple PRs to update names to pass the script

@maurelian
Copy link
Contributor Author

maurelian commented Dec 1, 2022

@smartcontracts OK.
See #4122 and #4124 for a start.

@maurelian maurelian closed this Dec 1, 2022
@maurelian maurelian deleted the jm/ctb-test-cleanup/base branch December 6, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants