Added A Test Policy #4908
Added A Test Policy #4908
Conversation
Samyoul
requested review from
ilmotta,
cammellos,
qfrank,
igor-sirotin and
osmaczko
Jenkins BuildsClick to see older builds (21)
|
_docs/policies/tests.md
Outdated
| - Prove that the functionality performs as described | ||
| - Can be falsified | ||
| - Are resistant to fuzzing | ||
| - All new tests MUST BE validated via 1000 local tests. Ensuring that the test runs and passes consistently every time gives confidence that the test is not flaky. |
There was a problem hiding this comment.
This sentence I sort of disagree. Sort of because perhaps we should differentiate the type of test we want to rerun hundreds of times because a good portion of unit tests are so deterministic in nature that this recommendation seems out of place or wasted effort to even worry about. Could we change the wording to something more flexible or realistic?
The tricky thing is that we haven't ever (or have we?) agreed on how to classify status-go's tests. If they all go in the same bucket as "tests" it becomes harder for us to express which ones we really want devs to increase -count.
There was a problem hiding this comment.
I guess we should say that integration tests should be rerun
There was a problem hiding this comment.
That is reasonable. Let's change the language to:
| - All new tests MUST BE validated via 1000 local tests. Ensuring that the test runs and passes consistently every time gives confidence that the test is not flaky. | |
| - All new integration tests MUST BE validated via at least 10000 local tests. Ensuring that the test runs and passes consistently every time gives confidence that the test is not flaky. |
I also agree with you @ilmotta we should have categories of test. Maybe we can open an issue and place a link to it in the Go Council page to mark this work as needing consensus.
There was a problem hiding this comment.
I'm also happy to add a definition of "integration tests."
@Samyoul make an issue and define
|
you will never merge this because tests fail 
|
_docs/policies/tests.md
Outdated
| - Prove that the functionality performs as described | ||
| - Can be falsified | ||
| - Are resistant to fuzzing | ||
| - All new tests MUST BE validated via 1000 local tests. Ensuring that the test runs and passes consistently every time gives confidence that the test is not flaky. |
There was a problem hiding this comment.
I guess we should say that integration tests should be rerun
There was a problem hiding this comment.
Thanks for this. I believe this is an ideal candidate for a flowchart (Mermaid). It will be much more readable. (Come on, we are developers 🤓 )
flowchart TB
A[Start: Before merging] --> B{Check rerun reports}
B -->|Yes| C[Check if test is in flaky tests GitHub issues or in the last three nightly test runs]
C -->|No| D[Flakiness likely introduced by your changes]
D --> E[Address flakiness]
E --> B
C -->|Yes| F[Add a comment to the existing issue or open a new issue if none exists]
B -->|No| G[Proceed with merge]
F --> G
^ It's merely a draft, needs more love. If you don't have time for it, I can come up with something more mature at the end of the week.
_docs/policies/tests.md
Outdated
| - Prove that the functionality performs as described | ||
| - Can be falsified | ||
| - Are resistant to fuzzing | ||
| - All new tests MUST BE validated via 1000 local tests. Ensuring that the test runs and passes consistently every time gives confidence that the test is not flaky. |
There was a problem hiding this comment.
Would be great to automate it and run on CI. What is not a CI check, will never be respected. Some will forget, some will not know (new contributors), some will not run to save time.
There was a problem hiding this comment.
I agree with your points. For now we can include it as part of the policy as this is the behaviour we want regardless of how we enforce the requirement.
I'll raise an issue to explore / implement the automation of "new integration test mega runs". I'll track it via the Go Council project board ... Which doesn't exist yet but we probably should have it as we have multiple improvements to track, and this improvement is dependent on being able to categorise certain tests as "integration test".
There was a problem hiding this comment.
@Samyoul make issue and somewhere to track Go Council related things
There was a problem hiding this comment.
Ok this issue is now done #4952
|
Thank you for all the feedback and so quickly. I will get back to you on each point. Also I should probably not have put so many jokes in the document. I'll fix that and turn this document into a serious policy without the memes and edgelording. Also 🤦 this is just perfection #4908 (comment) 
|
Samyoul
force-pushed
the
docs/tests-policy
branch
from
b611df7
to
6602d49
Compare
Samyoul
requested review from
rasom,
igor-sirotin,
osmaczko,
jrainville and
ilmotta
The changes of the PR are basically the description. This is based on a
status-gocrew meeting https://www.notion.so/Sync-Call-67878e2bdbe447ddab0ebc462be8e64fThe human readable policy is below:
status-goTest PolicyCreating Tests
integration testsMUST BE validated via a minimum of 1000 tests.-countor-test.countflag with the test command eg:-count 1000/-test.count 1000TODOAdd link to issue for CI automation of validation test runs of newintegration tests.Flaky Tests
Flaky tests are defined as tests that fail intermittently.
Steps to resolving or reporting flaky tests
Is it me?
Determine who caused the flaky test.
TODOadd link to rerun reportsflowchart TB A([PR ready for merge]) --> B{Have any test failed?} B -->|No| C[🎉 Proceed with merge 🪄] B -->|Yes| D{ Is the failing test introduced or altered by this PR? } D -->|No| E[Check rerun reports.] D -->|Yes| F[ It is likely your changes introduced the flakiness. You MUST fix the test before merge is acceptable. ] F --> A E --> G{Does the test appear in `E:Flaky Test` issues<br/> or in the last three nightly test runs?<br/>} G -->|Yes| I[The flakiness needs reporting] G -->|No| F I --> J([Proceed to Reporting flow])Reporting Flaky Tests
If an old test fails and/or seems flaky either locally or in CI, you MUST report the event.
status-goGitHub repo issues for the test name(s) failing.flowchart TB A([Ready to report a flaky test]) --> B[Check the `status-go` GitHub repo<br/>issues for the test name failing.] B --> C{Does the test appear in<br/>the list of `E: Flaky Test` issues?} C -->|No| D[ Create a new issue - The issue title should include the flaky test name - The issue should use the `E:Flaky Test` label ] D --> E[ Detail which test is flaky and in what context: local vs CI, link to the PR or branch. ] E --> J C -->|Yes| F{Is the issue open?} F -->|No| G((Either)) H --> E G --> I[Reopen the issue] G --> D I --> H F -->|Yes| H[Add a comment to the issue] J([End])