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

RFC: manifest file #1338

Merged
merged 6 commits into from
Jul 21, 2023
Merged
Changes from 4 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
134 changes: 134 additions & 0 deletions docs/edit/manifest.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
### 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
Copy link
Member

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.

Copy link
Collaborator Author

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.


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 team (each tracer, agent, ASM rules file, any php extension...), 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
- Manifest file will be validated using JSON schema in system tests CI
- An error will pop if a manifest file refers to a file/class that does not exists

## Supported features

Will support:

- 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:

- any complex logic
_ because there is not limit on the complexity. We need to draw a line based on the ratio format simplicity / number of occurrences. The cutoff point is only test classes, declaring version for weblog variants, or skip reason for the entire class.
- 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 ...
Copy link
Member

Choose a reason for hiding this comment

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

No version qualifier for bug/missing_feature, etc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that without proper tooling to validate these files, things may get worse. So let's keep an eye on tooling that helps getting things right, and not committing wrong manifests.

-> f09d34c

Having different ways to declare file-level and class-level (manifest file) and test-level (in code) sounds a bit like keeping some of the bad things of both worlds?

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 bug, flaky....). But as it did not complexify the data model, I thought it was a good middle point.


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
{
"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.*|\\?)" },

"skipped_declaration": {
"type": "string",
"pattern": "^(bug|flaky|irrelevant) \\(.*\\)$"
}
}
}
```