-
-
Notifications
You must be signed in to change notification settings - Fork 590
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
Formalise guidelines around writing tests #2179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typo - otherwise wholeheartedly agree!
Co-authored-by: Andy Balaam <andyb@element.io>
CONTRIBUTING.md
Outdated
2. "happy path" end-to-end tests. | ||
These are located in `/test/end-to-end-tests`. Ideally, you would also | ||
include tests for edge and error cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only applies to react-sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly wording clarifications - the principle is agreeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable! Some good points about the various different docs at different levels of the tree: we should try & clean these up a bit (I think I'm convinced that the right place for things to live would be element-web).
@@ -110,6 +110,13 @@ must include: | |||
These are located in `/test/end-to-end-tests`. Ideally, you would also | |||
include tests for edge and error cases. | |||
|
|||
Unit tests are expected even when the feature is in labs. It's good practice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most labs flags would want integration tests rather than unit tests, and the labs flags which would want to skip the tests would do so because of the highly experimental nature of that particular project. The example I have is matrix-org/matrix-react-sdk#7815 - we want to test the changes with the minimum possible investment to then make a decision about whether or not it survives into a formal release. For this sort of workflow we don't generally care if the labs behaviour breaks, but do obviously care if the non-labs behaviour breaks (which should hopefully be covered by pre-existing tests).
I think there's immense value in maintaining this sort of workflow, as long as we understand when and how to apply it. It should never be the common case that labs features land without tests, but it would be nice to have some room in the policy for them to exercise a more lean, highly iterative, workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we're saying some code can escape the threshold of typical quality, we need a more powerful distinction for marking it as "experimental" than just labs. A lot of labs code these days really is intended for eventual production, and skipping tests during development is problematic because adding tests later usually doesn't work - the code won't be very testable and will need reworking.
Added this for discussion with the Element Web team.
1. e2e tests live in matrix-react-sdk 2. we can make exceptions for experimental code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks :)
As discussed internally and in #element-dev
This PR currently has no changelog labels, so will not be included in changelogs.
A reviewer can add one of:
T-Deprecation
,T-Enhancement
,T-Defect
,T-Task
to indicate what type of change this is, or addType: [enhancement/defect/task]
to the description and I'll add them for you.