Refactored metadata linting #1851

happz · 2023-02-14T21:59:40Z

The existing user-facing API did not change, we still have the CLI commands we're used to, tmt [test|plan|story] lint and tmt lint. internally, things are not different, with linters split into categories, automagical discovery, formalized internal APIs, and new CLI look (--list-checks, --failed-only, --enable-checks & brand new output).

$ tmt story lint /spec/plans/import
/spec/plans/import
pass C000 fmf node passes schema validation
pass C001 summary key is set
pass C002 summary is reasonably long
pass S001 correct key are used
fail S002 story is required

Fixes #2006

thrix · 2023-03-07T11:09:27Z

@happz while running it manually, this looks weird:

warn C001 summary key is missing, it is useful for quick inspection
skip C002 summary key is missing, it is useful for quick inspection

thrix · 2023-03-07T11:10:01Z

@happz also we will need to add some documentation about this, describing all the available checks?

thrix

Some small comments, otherwise the code looks clean

tests/lint/test.sh

tmt/lint.py

happz · 2023-03-11T10:37:14Z

@happz also we will need to add some documentation about this, describing all the available checks?

@thrix --list-checks should help:

$ tmt lint --list-checks
Linters available for tests
C000: fmf node should pass the schema validation
C001: summary key should be set, it is very useful for quick inspection
C002: summary should not exceed 50 characters
T001: all keys are known
T002: test script must be defined
T003: test directory path must be absolute
T004: test directory path must exist
T005: relevancy has been obsoleted by adjust
T006: coverage has been obsoleted by link
T007: manual test path is not an actual path
T008: manual test should be valid markdown

Linters available for plans
C000: fmf node should pass the schema validation
C001: summary key should be set, it is very useful for quick inspection
C002: summary should not exceed 50 characters
P001: all keys are known
P002: execute step must be defined with "how"
P003: execute step methods must be known
P004: discover step methods must be known
P005: remote fmf ids must be valid

Linters available for stories
C000: fmf node should pass the schema validation
C001: summary key should be set, it is very useful for quick inspection
C002: summary should not exceed 50 characters
S001: all keys are known
S002: story key must be defined
$

tests/test/lint/test.sh

LecrisUT · 2023-05-05T08:53:20Z

A note from #2008, should add a test condition for when the fmf root is in a different path. Maybe that should be made a test fixture/conftest.py so that it can be added to all test.

happz · 2023-05-05T09:10:42Z

A test for #2006 added in fa2520b

psss · 2023-05-05T09:58:04Z

What about using a different color for the skip label? Currently it can be easily confused with pass.

LecrisUT · 2023-05-05T10:02:10Z

Green/Gray/Red/Yellow for pass/skip/fail/warning? These should be standard

psss · 2023-05-05T10:08:39Z

When an invalid key is present, the problem is reported twice:

warn C000 key "requires" not recognized by schema, and does not match "^extra-" pattern
fail T001 unknown key "requires" is used

Is this intentional? When two unknown keys are provided, duplication is gone:

warn C000 fmf node failed schema validation
warn C001 summary key is missing
fail T001 unknown key "hm" is used
fail T001 unknown key "requires" is used

Not sure why.

psss · 2023-05-05T10:10:08Z

Green/Gray/Red/Yellow for pass/skip/fail/warning? These should be standard

Sounds good to me. Maybe blue could be also suitable for skip as an "info" color.

happz · 2023-05-05T10:25:18Z

Green/Gray/Red/Yellow for pass/skip/fail/warning? These should be standard

Sounds good to me. Maybe blue could be also suitable for skip as an "info" color.

Changed to white in 5b356c5 as Click does not offer gray. Let me know whether it should be blue instead...

happz · 2023-05-05T10:32:17Z

When an invalid key is present, the problem is reported twice:
warn C000 key "requires" not recognized by schema, and does not match "^extra-" pattern
fail T001 unknown key "requires" is used
Is this intentional? When two unknown keys are provided, duplication is gone:
warn C000 fmf node failed schema validation
warn C001 summary key is missing
fail T001 unknown key "hm" is used
fail T001 unknown key "requires" is used
Not sure why.

I do know why, fixed in e2169ff.

tests/usability/main.fmf

psss · 2023-05-05T13:45:59Z

Changed to white in 5b356c5 as Click does not offer gray. Let me know whether it should be blue instead...

Thanks. Just tried with my light terminal and the skip tags are hardly readable :( Let's go with blue instead.

happz · 2023-05-05T13:50:34Z

Changed to white in 5b356c5 as Click does not offer gray. Let me know whether it should be blue instead...

Thanks. Just tried with my light terminal and the skip tags are hardly readable :( Let's go with blue instead.

Changed to blue in 8fc1e89

psss

This is a great enhancement! And will be superuseful. Quite an elegant implementation! Found some minor issues, a couple of questions, nothing serious.

tmt/cli.py

tmt/lint.py

tmt/base.py

psss

Thanks for addressing all comments!

The existing user-facing API did not change, we still have the CLI commands we're used to, `tmt [test|plan|story] lint` and `tmt lint`. internally, things are not different, with linters split into categories, automagical discovery, formalized internal APIs, and new CLI look (`--list-checks`, `--failed-only`, `--enable-checks` & brand new output). ``` $ tmt story lint /spec/plans/import /spec/plans/import pass C000 fmf node passes schema validation pass C001 summary key is set pass C002 summary is reasonably long pass S001 correct key are used fail S002 story is required ```

Should be covered.

psss · 2023-05-09T08:40:44Z

/packit test

psss · 2023-05-09T10:28:36Z

The only failure is /tests/finish/ansible error on fetching the centos:7 image. Happened multiple time recently. Filed as #2063.

happz force-pushed the lint-checks-refactored branch 3 times, most recently from d1b1ad0 to 9e7e9db Compare February 15, 2023 11:50

happz marked this pull request as ready for review February 15, 2023 11:50

happz requested review from psss, FrNecas and lukaszachy as code owners February 15, 2023 11:50

happz added the command | lint tmt lint command label Feb 15, 2023

happz force-pushed the lint-checks-refactored branch from 9e7e9db to 62168c5 Compare February 15, 2023 11:58

happz force-pushed the common-base-class-and-call-other-branch branch from 5b15c59 to e00562a Compare February 17, 2023 12:37

psss force-pushed the common-base-class-and-call-other-branch branch from c2b6dfb to 9e112d7 Compare February 20, 2023 17:14

Base automatically changed from common-base-class-and-call-other-branch to main February 20, 2023 18:29

happz force-pushed the lint-checks-refactored branch from 62168c5 to 3928c8c Compare February 21, 2023 09:47

happz added this to the 1.23 milestone Feb 22, 2023

happz force-pushed the lint-checks-refactored branch from 3928c8c to dccf040 Compare February 28, 2023 14:55

thrix requested review from thrix and janhavlin March 3, 2023 13:10

thrix previously requested changes Mar 7, 2023

View reviewed changes

tests/lint/test.sh Outdated Show resolved Hide resolved

tmt/lint.py Outdated Show resolved Hide resolved

janhavlin reviewed Mar 7, 2023

View reviewed changes

tmt/lint.py Outdated Show resolved Hide resolved

happz force-pushed the lint-checks-refactored branch from dccf040 to 80ccae2 Compare March 11, 2023 10:35

happz force-pushed the lint-checks-refactored branch 2 times, most recently from 4176671 to 045840d Compare March 28, 2023 09:07

happz mentioned this pull request Mar 29, 2023

Allow pre-commit lint hook to hide successful linting #1819

Closed

happz force-pushed the lint-checks-refactored branch from 045840d to 99cee20 Compare March 29, 2023 10:35

happz force-pushed the lint-checks-refactored branch from 22d5524 to 0b0fcba Compare April 13, 2023 07:07

github-advanced-security bot found potential problems Apr 13, 2023

View reviewed changes

happz force-pushed the lint-checks-refactored branch from 0b0fcba to 68ada59 Compare April 17, 2023 07:39

happz force-pushed the lint-checks-refactored branch from ae17a8a to 476dc2a Compare May 4, 2023 08:59

lukaszachy approved these changes May 4, 2023

View reviewed changes

happz force-pushed the lint-checks-refactored branch from 476dc2a to 1221a2a Compare May 5, 2023 07:22

happz mentioned this pull request May 5, 2023

Fix #2006 #2008

Closed

happz mentioned this pull request May 5, 2023

tmt lint fails with fmf path not in root #2006

Closed

happz force-pushed the lint-checks-refactored branch from 1221a2a to fa2520b Compare May 5, 2023 09:10

happz mentioned this pull request May 5, 2023

Allow pre-commit to run in subpath #2007

Merged

psss reviewed May 5, 2023

View reviewed changes

tests/usability/main.fmf Outdated Show resolved Hide resolved

happz force-pushed the lint-checks-refactored branch from 20985c1 to 8fc1e89 Compare May 5, 2023 13:49

psss requested a review from thrix May 5, 2023 13:52

psss self-assigned this May 5, 2023

psss requested changes May 5, 2023

View reviewed changes

psss approved these changes May 9, 2023

View reviewed changes

psss force-pushed the lint-checks-refactored branch from 21c4e01 to 68276b9 Compare May 9, 2023 06:11

psss merged commit 68276b9 into main May 9, 2023

psss deleted the lint-checks-refactored branch May 9, 2023 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored metadata linting #1851

Refactored metadata linting #1851

happz commented Feb 14, 2023 •

edited by psss

Loading

thrix commented Mar 7, 2023

thrix commented Mar 7, 2023

thrix left a comment

happz commented Mar 11, 2023

LecrisUT commented May 5, 2023

happz commented May 5, 2023

psss commented May 5, 2023

LecrisUT commented May 5, 2023

psss commented May 5, 2023

psss commented May 5, 2023

happz commented May 5, 2023

happz commented May 5, 2023

psss commented May 5, 2023

happz commented May 5, 2023

psss left a comment

psss left a comment

psss commented May 9, 2023

psss commented May 9, 2023

Refactored metadata linting #1851

Refactored metadata linting #1851

Conversation

happz commented Feb 14, 2023 • edited by psss Loading

thrix commented Mar 7, 2023

thrix commented Mar 7, 2023

thrix left a comment

Choose a reason for hiding this comment

happz commented Mar 11, 2023

LecrisUT commented May 5, 2023

happz commented May 5, 2023

psss commented May 5, 2023

LecrisUT commented May 5, 2023

psss commented May 5, 2023

psss commented May 5, 2023

happz commented May 5, 2023

happz commented May 5, 2023

psss commented May 5, 2023

happz commented May 5, 2023

psss left a comment

Choose a reason for hiding this comment

psss left a comment

Choose a reason for hiding this comment

psss commented May 9, 2023

psss commented May 9, 2023

happz commented Feb 14, 2023 •

edited by psss

Loading