-
Notifications
You must be signed in to change notification settings - Fork 13
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
RFC: manifest file #1338
RFC: manifest file #1338
Changes from 1 commit
ad12991
f09d34c
f65626e
ecda023
c9c7b80
b1bd632
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
### Context | ||
|
||
The usual way to declare what will be tested or not are decorators (`@released`, `@bug`, `@missing_feature`...). This solution offers several advantages : | ||
|
||
- declarations are as close as possible to the test, making the link obvious | ||
- and complex condition (several components involved, complex version range...) is easy to do, as it's declared as python code | ||
|
||
### Issue | ||
|
||
Unfortunatly, it comes with a major drawback: Activating a single test can become a hell due to confilct between PRs. | ||
|
||
### Proposition | ||
|
||
To solve this, we'll try to leverage two points : | ||
|
||
1. Often, those declarations involve only one component | ||
2. Often, a component is modified by only one developper | ||
|
||
The idea is to offer an alternative way to declare those metadata, using one file per component, also known as manifest file. Those files will be inside system tests repo, declaring for a given component some metadata. | ||
|
||
Pro : | ||
|
||
- Will solve the majority of conflict during test activation PRs | ||
- each team will have ownership of its manifest file, easing the PR preview process | ||
|
||
Cost : | ||
|
||
- declarations will be a little bit far away from the test, asking a small addition in the learning curve | ||
- re-naming will be a little bit harder, and will spam all teams | ||
|
||
## Implementation | ||
|
||
- Manifests files will be inside `utils/manifest/`, one per term, named with the argument name used in decorators (`utils/manifest/golang.yml`, `utils/manifest/agent.yml`...) | ||
- Each team will have ownership of its manifest file | ||
- pytest will load those files, and decorate tests node during collection | ||
|
||
## Supported features | ||
|
||
Will suport: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO this should land together with a script that:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. -> f09d34c There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's the dashboard job :) |
||
|
||
- declaring a skip reason (bug, flaky, irrelevant, missing_feature) for entire file | ||
- declaring released version (or a skip reason) for a class | ||
- declaring released version (or a skip reason) for a class, with details for weblog variants | ||
- The wildcard `*` is supported for weblog declarations | ||
|
||
Won't support: | ||
simon-id marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- declaring metadata (bug, flaky, irrelevant) for test methods | ||
- because their namings are not stable, it would lead to frequent modifications of manifest files, spaming every team | ||
- because conflict mostly happen at class level | ||
|
||
|
||
## Example | ||
|
||
```yaml | ||
tests/: | ||
specific.py: irrelevant (see this link) # let skip an entire file | ||
|
||
appsec/: # more regular declarations: | ||
test_distributed.py: | ||
Test_FeatureA: v1.14 # declare a version for a class | ||
|
||
Test_FeatureB: flaky # skip a class with bug, flaky, irrelevant ... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No version qualifier for bug/missing_feature, etc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
-> f09d34c
It's a compromise. The main issue with tests methods is that their names change a lot. Every time you change a test method name, you'll need to update all manifest files, spamming all teams. Lot of people complain already about being over notified by system tests. And as you can only have one declaration per component at method level, conflict is more managable. Actually, I even hesitated to only allow version declaration in the manifest (no |
||
|
||
Test_FeatureC: # declare a version for a class, depending on weblog | ||
django: v1.2 | ||
flask: v1.3 | ||
uwsgi: bug (jira ticket) # For a weblog, skip it with bug, or flaky | ||
"*": "?" # All other weblogs: not yet available | ||
``` | ||
|
||
## Format [WIP] | ||
|
||
```json | ||
{ | ||
cbeauchesne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"type": "object", | ||
|
||
"properties": { | ||
"tests/": { "$ref": "#/$defs/folder_content" } | ||
}, | ||
|
||
"$defs": { | ||
"folder_content": { | ||
"type": "object", | ||
"patternProperties": { | ||
".*/": { "$ref": "#/$defs/folder_content" }, | ||
"test_.*\\.py": { "$ref": "#/$defs/file_content" } | ||
} | ||
}, | ||
|
||
"file_content": { | ||
"oneOf": [ | ||
{ | ||
"type": "object", | ||
"patternProperties": { | ||
"Test_.*": { | ||
"oneOf": [ | ||
{ "$ref": "#/$defs/version" }, | ||
{ | ||
"type": "object", | ||
"patternProperties": { | ||
".*": { "$ref": "#/$defs/class_content" } | ||
} | ||
} | ||
] | ||
} | ||
} | ||
}, | ||
{ | ||
"$ref": "#/$defs/skipped_declaration" | ||
} | ||
] | ||
}, | ||
|
||
"class_content": { | ||
"oneOf": [ | ||
{ "$ref": "#/$defs/version" }, | ||
{ "$ref": "#/$defs/skipped_declaration" } | ||
] | ||
}, | ||
|
||
"version": { " type": "string", "pattern": "(v.*|\\?)" }, | ||
cbeauchesne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
"skipped_declaration": { | ||
"type": "string", | ||
"pattern": "^(bug|flaky|irrelevant) \\(.*\\)$" | ||
} | ||
} | ||
} | ||
``` |
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'm thinking we just avoid renaming tests mostly, and update comments instead to better reflect test changes if needed.
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.
Yes, as folders and test class names are the identifier of features, renaming are rare, so it's why I think it's acceptable.