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

feat: Add testing contribution guideline #229

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/10-contribute.md → docs/10-contribute/01-contribute.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Serverpod is built by the community for the community. Pull requests are very mu
<div style={{ position : 'relative', paddingBottom : '56.25%', height : '0' }}><iframe style={{ position : 'absolute', top : '0', left : '0', width : '100%', height : '100%' }} width="560" height="315" src="https://www.youtube-nocookie.com/embed/V3CqPx4jykE" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe></div>

## Roadmap
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: A redirect needs to be implemented to catch any links to the old contribution guidelines.

Also - do we reference this from the Serverpod repo? If so, the link there needs to be updated as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also recently flattened out this structure (creating the Roadmap & contributions menu item). Are we sure we want to nest it again?


If you want to contribute, please view our [roadmap](https://github.com/orgs/serverpod/projects/4) to make sure your contribution is in-line with our plans for future development. This will make it much more likely that we can include the new features you are building. You can also check our list of [good first issues](https://github.com/serverpod/serverpod/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22).

:::important
Expand Down Expand Up @@ -109,7 +110,6 @@ If you are contributing new code, you will also need to provide tests for your c

Feel free to post on [Serverpod's discussion board](https://github.com/serverpod/serverpod/discussions) if you have any questions. We check the board daily.


## Repository overview

Serverpod is a large project and contains many parts. Here is a quick overview of how Serverpod is structured and where to find relevant files.
Expand Down Expand Up @@ -142,4 +142,4 @@ These are 1st party modules for Serverpod. Currently, we maintain an authenticat

### `integrations`

These are integrations for 3rd party services, such as Cloud storage.
These are integrations for 3rd party services, such as Cloud storage.
258 changes: 258 additions & 0 deletions docs/10-contribute/02-testing-guideline.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,258 @@
# Serverpod Testing Philosophy

## Overview

At Serverpod, our core testing philosophy revolves around achieving the following goals:

- **Readable and Self-Explanatory Tests** – Tests should be easy to understand at a glance. Descriptions must clearly convey the purpose and expected outcome without needing to inspect the test's internal implementation.
- **Resilient to Refactoring** – Tests should not break due to internal refactoring. As long as the external behavior remains consistent, tests should pass regardless of code structure changes.
- **Focused on Behavior, Not Implementation** – We prioritize testing how the code behaves rather than how it is implemented. This prevents unnecessary coupling between tests and production code, fostering long-term stability.
- **Easy to Maintain and Expand** – Tests should be simple to update or extend as the product evolves. Adding new features should not require widespread changes to existing tests.
- **Effective at Catching Bugs** – The primary goal of testing is to identify and prevent bugs. Our tests are crafted to cover edge cases, ensure proper functionality, and catch potential regressions.

By adhering to the following principles, we ensure that our test suite remains a valuable and reliable asset as our codebase grows.

This document outlines Serverpod's approach to testing code. It serves as a guide for writing effective, maintainable, and meaningful tests across all our projects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Move this paragraph to before the ## Overview heading


## Key Principles
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Use capital letter only on the first word.

This needs to be applied to all headers.


### 0. Test Independence
Copy link
Contributor

Choose a reason for hiding this comment

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

Question/Discussion: In written text, the first item is often 1. Any particular reason for having 0 as the first rule here?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do we do in the rest of the documentation?


- **Tests should be completely independent of one another.**
- The outcome of a test must never depend on any other test running before or after it.
- The order in which tests are executed **should not matter.**
- Running a single test in isolation must produce the same result as running it alongside others.
- **Exception to the rule:** e2e and integration tests. In scenarios where an external state (like a shared database) is involved, tests may require concurrency mode 1 to prevent interference. But each test should start and end in a clean state.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two kinds of independence that can be worth while distinguishing:

  • technical independence (reentrancy), which may not be possible with external resources such as DB, in which case non-concurrent execution is required.
  • semantic independence, which should only be broken when the tests specifically test a sequence of operations (scenario tests / test scripts). This test scripts need to be "atomic" - they can't be subdivided or change internal order of execution, but should still be independent of each other.

Suggestion: Move the last "exception" point to a separate elaboration, tests ought to be written to be independent even if there are technical dependencies (currently... might be improved!)


### 1. Clear and Descriptive Test Descriptions

- **Test descriptions should be understandable without reading the test code.**
- If a test fails, the description alone should make it clear what went wrong.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add behavior: "make it clear what behavior went wrong"

- **Format:** Descriptions follow the "Given, When, Then" style.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion/Discussion: Should we outline what the different words mean?


**Example:**

```dart
// Given a user with insufficient permissions
// When attempting to access a restricted page
// Then a 403 Forbidden error is returned
```

### 2. Focus on Single Responsibility

- Each test should **only test one thing**.
- **Avoid** bundling multiple independent checks into a single test.
- It is acceptable to **repeat the same precondition and action** across tests to ensure each aspect is tested individually.

**Example:**

```dart
// Good - Tests are split
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Use the same Don't and Do pattern used when explaining best practises for our testing framework to ensure consistency in the documentation.

test('Given an empty list when a string is added then it appears at the first index', () {
final list = [];
list.add('hello');
expect(list[0], 'hello');
});

test('Given an empty list when a string is added then list length increases by one', () {
final list = [];
list.add('hello');
expect(list.length, 1);
});

// Bad - Multiple independent checks in one test
test('Add string to list and check index and length', () {
final list = [];
list.add('hello');
expect(list[0], 'hello');
expect(list.length, 1);
});
```

- Multiple `expect` statements are not necessarily against this rule. However, they must check values that are interdependent and only meaningful when evaluated together.

**Example:**

```dart
test('Given a missing semicolon when validated then the entire row is highlighted', () {
final code = 'final a = 1';
final result = validateCode(code);

expect(result.span.start.column, 1);
expect(result.span.end.column, 12);
expect(result.span.start.line, 1);
expect(result.span.end.line, 1);
});
```

- In this case, verifying both the start and end positions of the `SourceSpanException` is essential because they collectively describe the error location, and their correctness is interdependent.

\*Note: SourceSpanException is an object that describes a code error in a source file. See: \*[*https://api.flutter.dev/flutter/package-source\_span\_source\_span/SourceSpanException-class.html*](https://api.flutter.dev/flutter/package-source_span_source_span/SourceSpanException-class.html)&#x20;

### 3. Pure Unit Testing

- **Unit tests should avoid mocking and side effects.**
- Production code should push side effects **up the call stack**, allowing tests to cover pure methods.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add definition of "pure" to this section, since this is for a public audience and not everyone knows exactly what it is, e.g.

Pure functions

Pure functions are simply put functions that lack side effects, lack external I/O, and always have the same results for the same inputs. See https://en.wikipedia.org/wiki/Pure_function

Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to rephrase this completely, maybe focus more on the non-side effect part. Since Unit tests have different definitions as well. Or we should explain what we view as a Unit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be good to break them apart even? One that outlines the test abstraction level and one that focuses on side effects? This might already be covered in point 4. Implementation-agnostic tests.

- **Test the logical feature/unit** rather than a single method or class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add "Prefer" to the last point:

  • Prefer to test the logical feature/unit rather than each single method or class.


**Example:**

```dart
// Avoid mocking HTTP requests directly, test the logic that processes the response
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: add a do and don't to this section as well.

Future<String> fetchUserData() async {
final response = await httpClient.get(url);
return processResponse(response);
}
```

- Test `processResponse` directly without mocking the `httpClient`.

### 4. Implementation-Agnostic Tests

- **Do not couple tests to implementation details.**
- Tests should only break if the behavior changes, not when refactoring code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Since this is sometimes referred to as black box testing, add a point to spell it out:

  • This may be referred to as black box testing.

- **Unit tests must avoid knowledge of internal methods or variables.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion/Discussion: Should we add a point that if internal details needs to be tested these are probably justified to be broken out into their own library gaining a public interface that is promised?

- The use of `@visibleForTesting` is discouraged as it exposes internal details that should remain hidden. This annotation can lead to brittle tests that break during refactoring, even if external behavior remains the same.

**Example:**

```dart
// Good - Tests against behavior
String processUserData(String rawData) {
return rawData.toUpperCase();
}

test('Given a user name when processed then the name is capitalized', () {
final result = processUserData('john');
expect(result, 'JOHN');
});

// Bad - Tests against internal methods or state
class UserDataProcessor {
String process(String rawData) {
return _toUpperCase(rawData);
}

@visibleForTesting
String toUpperCase(String input) => input.toUpperCase();
}

// Bad - Verifying internal method call
test('Given a user name when processed then toUpperCase is called', () {
final processor = UserDataProcessor();

when(() => processor.toUpperCase('john')).thenReturn('JOHN');

final result = processor.process('john');

expect(result, 'JOHN');
verify(() => processor.toUpperCase('john')).called(1);
});
```

- In the bad example, the test verifies that an internal method (`_toUpperCase`) is called, coupling the test to the implementation. The good example validates only the output, ensuring the test focuses on behavior rather than internal details.

### 5. Immutable Test Philosophy

- **Tests should rarely be modified.**
- When functionality changes, **add new tests** and/or **remove obsolete ones**.
- Avoid altering existing tests unless fixing bugs, e.g. invalid tests.

### 6. Simplicity Over Abstraction

- The explicit examples make it clear what is being tested and reduce the need to jump between methods. The abstracted logic example hides test behavior, making it harder to understand specific test scenarios at a glance.
- **Avoid abstraction in tests.**
- Tests should be **simple, explicit, and easy to read.**
- Logic or shared test functionality should be **copied** rather than abstracted.

**Examples:**

```dart
// Good - Explicit test
test('Given a number below threshold when validated then an error is collected', () {
final result = validateNumber(3);

expect(result.errors.first.message, 'Number must be 5 or greater.');
});

// Good - Explicit test for out of range value
test('Given a number above range when validated then range error is collected', () {
final result = validateNumber(11);

expect(result.errors.first.message, 'Number must not exceed 10.');
});

// Bad - Abstracted logic into a method
void performValidationTest(int number, String expectedErrorMessage) {
final result = validateNumber(number);

expect(result.errors.first.message, expectedErrorMessage);
}

test('Number below threshold', () {
performValidationTest(3, 'Number must be 5 or greater.');
});

test('Number above range', () {
performValidationTest(11, 'Number must not exceed 10.');
});
```

### 7. Beneficial Abstractions - Test Builders
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion/Discussion: Should we mention factories here as well? When the built objects are dependent on each other?


- One abstraction we encourage is the **test builder pattern** for constructing test input objects.
- Builders help create logical, valid objects while keeping tests simple and readable.

**Key Characteristics:**

- Builder class names follow the pattern: `ObjectNameBuilder`.
- Methods that set properties start with `with`, and return the builder instance (`this`).
- Builders always include a `build` method that returns the final constructed object.
- Constructors provide reasonable defaults for all properties to ensure valid object creation by default.

**Guidelines:**

- In tests, always set the properties that are important for the specific scenario.
- Defaults allow seamless addition of new properties without modifying existing tests.
- Using builders ensures objects are created in a realistic, lifecycle-accurate way.

**Example:**

```dart
class UserBuilder {
String _name = 'John Doe';
int _age = 25;

UserBuilder withName(String name) {
_name = name;
return this;
}

UserBuilder withAge(int age) {
_age = age;
return this;
}

User build() {
return User(name: _name, age: _age);
}
}

// Test Example
test('Given a user at the legal cut of when checking if they are allowed to drink then they are', () {
final user = UserBuilder().withAge(18).build();
final isAllowed = isLegalDrinkingAge(user);
expect(isAllowed, isTrue);
});
```

- **Benefits:**
- Reduces test brittleness when refactoring.
- Ensures tests continue to produce valid objects as the code evolves.
- Simplifies object creation without requiring deep lifecycle knowledge.

## Final Thoughts

Consistent application of these principles leads to a robust and maintainable codebase, fostering confidence in our product's reliability and scalability.

We acknowledge that there are exceptions to the rules. But when deviating from the guidelines mentioned above you have to argue for why. You must have a reasonable argument that aligns with the core goals.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Remove double space between "there are"

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Change to: We acknowledge that there are exceptions to the rules. However, any deviation from the guidelines outlined above must be supported by a well-reasoned argument that aligns with the core objectives.

Loading