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

Conversation

Isakdl
Copy link
Contributor

@Isakdl Isakdl commented Jan 2, 2025

Changes

Adds contribution guideline for how we write tests at Serverpod including examples and explanation of our best pracitices.

Copy link
Contributor

@christerswahn christerswahn left a comment

Choose a reason for hiding this comment

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

Well worded and explicit! Wrote a few suggestions.


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

- 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!)

docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved

- **Unit tests should avoid mocking and side effects.**
- Production code should push side effects **up the call stack**, allowing tests to cover pure methods.
- **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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is a prefer thing, but a hard rule. We could define "logical feature/unit" a little more in detail however. A unit could be a single method, but most often it is not.

### 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.

docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved
docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved
Copy link
Contributor

@SandPod SandPod left a comment

Choose a reason for hiding this comment

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

Nice writeup! I agree with all of it and only have some minor comments and suggestions around structure and phrasing.

I'm not sure we should actually add this to our documentation thoe. Maybe it would make more sense to place this in the actual project and keep the simple Roadmap and Constributions here?

Then we could also have information on how to run tests there as well?
I'm not sure it makes sense to have information specific to "contributors" outside of the project? What do you think?

docs/10-contribute/01-contribute.md Show resolved Hide resolved
docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved
docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved
### 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.

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.

**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.

docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved
});
```

### 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?

docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved
docs/10-contribute/02-testing-guideline.md Show resolved Hide resolved
### 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.

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.

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