Skip to content

Test Clean Up#4084

Closed
maurelian wants to merge 12 commits intodevelopfrom
jm/ctb-refactor-tests
Closed

Test Clean Up#4084
maurelian wants to merge 12 commits intodevelopfrom
jm/ctb-refactor-tests

Conversation

@maurelian
Copy link
Contributor

@maurelian maurelian commented Nov 24, 2022

Description

Furthers the work of reorganizing the forge tests, which was begun in #3611.

Test organizing principles

  1. Uses contracts to organize the test suite similar to how mocha uses describe.
  2. Every non-trivial state changing function should have a contract for happy and sad path tests. This helps to make it very obvious where there are not sad tests (I haven't added any new tests, so there are quite a few empty placeholder contracts).
  3. Simpler functions like getters and setters are grouped together into test contracts.
  4. All contracts and functions should be consistently named following a convention.

Test naming convention

Test function names are split by underscores, into 3 or 4 parts. An example function name is test_onlyOwner_callerIsNotOwner_reverts().

The parts are: [method]_[FunctionName]_[reason]_[success], where:

  1. [method] is either test or testFuzz
  2. [FunctionName] is the name of the function or higher level behavior being tested.
  3. [reason] is an optional description for the behavior being tested.
  4. [success] must be one of success or reverts or _fails.

Name validation

I also wrote a quick and dirty script which can be run to validate function names and identify ones which are not yet compliant. From within the bedrock package:
ts-node scripts/forges-test-names.ts.

@changeset-bot
Copy link

changeset-bot bot commented Nov 24, 2022

⚠️ No Changeset found

Latest commit: ce6e397

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Nov 24, 2022
@maurelian maurelian changed the title Jm/ctb refactor tests Test Clean Up Nov 24, 2022
@maurelian maurelian force-pushed the jm/ctb-refactor-tests branch 2 times, most recently from cd20caf to 0bea1e4 Compare November 28, 2022 16:00
@maurelian maurelian marked this pull request as ready for review November 28, 2022 16:00
@maurelian maurelian force-pushed the jm/ctb-refactor-tests branch 3 times, most recently from b1b3764 to 8a97472 Compare November 28, 2022 17:17
@mergify
Copy link
Contributor

mergify bot commented Nov 28, 2022

Hey @maurelian! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Nov 28, 2022
@maurelian maurelian force-pushed the jm/ctb-refactor-tests branch from 8a97472 to ce6e397 Compare November 28, 2022 17:28
@mergify mergify bot removed the conflict label Nov 28, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2022

Hey @maurelian! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Nov 29, 2022
@maurelian
Copy link
Contributor Author

Closing will break up.

@maurelian maurelian closed this Nov 30, 2022
@mergify mergify bot removed the conflict label Nov 30, 2022
@maurelian maurelian deleted the jm/ctb-refactor-tests 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

A-pkg-contracts-bedrock Area: packages/contracts-bedrock

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant