Skip to content
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

Update unit test naming guidelines #134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented Mar 7, 2025

Test Naming Guidelines Update

Updates our test naming conventions to improve clarity and maintainability of our test suite.

Key Changes

  • Established clear guidelines for naming unit tests
  • Added examples of good and bad practices
  • Introduced principles for more descriptive and concise test names

@NicolasMassart NicolasMassart added the enhancement New feature or request label Mar 7, 2025
@NicolasMassart NicolasMassart self-assigned this Mar 7, 2025
@NicolasMassart NicolasMassart requested a review from a team as a code owner March 7, 2025 18:19
@NicolasMassart NicolasMassart changed the title update unit test naming guidelines Update unit test naming guidelines Mar 7, 2025
- Including implementation details in the name
- Stating obvious successful outcomes
- Listing test parameters instead of what they represent
- Using words like "fail", "error", or "throw" when the error is the expected behavior
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this. Sometimes if we want to test validation behaviour for example, I'd write something like this:

it('throws an error when ...', () => {
  // ...
});

Copy link
Contributor

Choose a reason for hiding this comment

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

I would agree with this. I understand behavior can be described different ways, and sometimes maybe it's enough to be more abstract in the description. But I do find it useful, on the unit level, to include some implementation details in the description, to be able to quickly understand the behavior. So this would be, what the unit under test a) returns, b) throws, or c) calls (and, if appropriate, in the context of a scenario such "...when passed A" or "...when B is set").

Copy link
Contributor Author

@NicolasMassart NicolasMassart Mar 11, 2025

Choose a reason for hiding this comment

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

Why does an error get thrown? Because the behavior is unexpected.
Instead of writing "the car running fails", simply write "the car does not run". You can add context: "the car does not run at night."

To understand the test details, read the test itself. This keeps names uncluttered and encourages clear, self-documenting test implementations. Too many tests are overly complex and require prior knowledge of the feature—this should be the reverse.

Also, if the implementation changes but the behavior remains, you won’t need to rename the test. A good test name, like a function name, shouldn’t be tied to implementation details.
For example, "the car runs" remains valid even if new expectations are added, such as checking that the airbag is on, tires are inflated, or the car has fuel. This avoids unnecessary name changes and keeps test reports readable.

2. Focus on what is being tested, not how
3. Be explicit about the context when needed
4. Keep names concise but descriptive
5. Avoid "should", "when", and other unnecessary words
Copy link
Contributor

@mcmire mcmire Mar 7, 2025

Choose a reason for hiding this comment

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

Avoid "should" makes sense, but I do find it useful to highlight under which condition the behavior under test applies, if any, as it's easy to then cross-check the test with the code being tested. Is this something you encounter or do you find that you do not need that information in the test name?

Copy link
Contributor Author

@NicolasMassart NicolasMassart Mar 11, 2025

Choose a reason for hiding this comment

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

The conditions should be clear in the test itself—use well-named constants for initialization. That’s part of the implementation.

What matters in the name is describing what is being tested, not how.

For example, if you're testing "the car works", the test will check that the engine starts, wheels turn, steering functions, and brakes slow the car. But the name remains "the car works", not a long list of checks.

If you add conditions, they should enhance clarity, e.g., "the car works at night" to test the lights—without listing every individual check.

Naming isn’t always easy (it's one of the hardest parts of programming), but with practice, it becomes more natural. In the long run, this approach improves knowledge sharing and understanding of the codebase.


### Examples

🚫
🚫 Don't
Copy link
Contributor

Choose a reason for hiding this comment

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

Something I've been doing for other guidelines is describe why something is good or bad. I didn't do it here but maybe we want to fix that? So, perhaps we could break up these examples into pairs. Something like:

**🚫 "should" is used in the test name**

```typescript
it('should successfully add token when address is valid and decimals are set and symbol exists', () => {
  // ...
});
```

**🚫 The test name describes the behavior as though it already exists**

```typescript
it('successfully adds token when address is valid and decimals are set and symbol exists', () => {
  // ...
});
```

...and then following with the next pair of examples, and the next, and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried to shorten things with combined examples, but I agree we could have more pairs for clarity. Will work on that.


```typescript
describe('TokensController', () => {
it('addToken', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should still include this example or something like it. This is something I've seen in some really old tests, where people don't bother to write a description for the thing they're testing. Then again, maybe it's obvious, I don't know. Just thought I'd point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes definitely, will work on this

@NicolasMassart NicolasMassart requested a review from Copilot March 11, 2025 13:27

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates our unit test naming guidelines to improve clarity and maintainability of our test suite.

  • Added a structured set of instructions for naming tests using active verbs.
  • Included new examples of both poor ("Don't") and good ("Do") naming practices.
  • Provided an extended list of considerations to ensure test names are descriptive and concise.
Comments suppressed due to low confidence (1)

docs/testing/unit-testing.md:125

  • [nitpick] The test name in this example includes the 'should' prefix even though the guidelines recommend avoiding it. Consider revising the test name to remove the 'should' prefix and focus directly on the behavior.
it('should successfully add token when address is valid and decimals are set and symbol exists', () => {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

3 participants