RFC: manifest file #1338
RFC: manifest file #1338
Conversation
cbeauchesne
force-pushed
the
cbeauchesne/rfc-manifest
branch
3 times, most recently
from
57170b4 to
daf41ef
Compare
cbeauchesne
force-pushed
the
cbeauchesne/rfc-manifest
branch
from
daf41ef to
ad12991
Compare
There was a problem hiding this comment.
The idea sounds good, however:
- 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.
- 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?
| 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.
No version qualifier for bug/missing_feature, etc?
There was a problem hiding this comment.
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.
docs/edit/manifest.md
Outdated
|
|
||
| ## Supported features | ||
|
|
||
| Will suport: |
There was a problem hiding this comment.
IMHO this should land together with a script that:
- Checks if manifest files are in sync with tests (e.g. not referencing missing classes).
- Ideally, also a way to add missing entries based on tests (or a way to list tests disabled for a language).
There was a problem hiding this comment.
or a way to list tests disabled for a language
It's the dashboard job :)
|
Is file level skipping a good idea ? the problem imo is that you might and up skipping test classes that are written after the file is skipped. If we skip classes individually instead of whole files, at least we have a chance to notice new tests in XPASS results for example. |
Great power, great responsabilities :) The use case for that is C++ that does not have any ASM feature, and I would be please if can avoid this ugly skip: if context.library == "cpp":
pytestmark = pytest.mark.skip("not relevant")But I agree, it should not be used with |
Co-authored-by: simon-id <[email protected]>
It's actually the continuation of its work (but I chose a different implementation, using the capacity of pytest to skip tests on the fly) |
| "type": "object", | ||
| "patternProperties": { | ||
| "Test_.+": { | ||
| "$comment": "It can be a version number, a skip reason, or an object with weblog variant as keys", |
There was a problem hiding this comment.
Is this comment field for each language team to write, for example, "this test is skipped until PR #123 is released", or for the test file declaration?
There was a problem hiding this comment.
in json-schema, $comment means a comment in the schema itself (as JSON does not allow comments).
The way to add a comment will be something like :
Test_Feature: bug (JIRA-666)
| 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 |
There was a problem hiding this comment.
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.
Yes, as folders and test class names are the identifier of features, renaming are rare, so it's why I think it's acceptable.
This PR will be used for the very first iteration of manifest file.
The goal is to provide a alternative way to declare versions, using one file per components.