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

JSON & XML formaters #442

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

QuentinN42
Copy link

@QuentinN42 QuentinN42 commented Feb 3, 2022

Add json and junitxml output format.

@QuentinN42 QuentinN42 mentioned this pull request Feb 3, 2022
@QuentinN42
Copy link
Author

Trough this point there is only some refactors.
All tests still pass (except test_run_with_user_global_config_file (tests.test_cli.CommandLineTestCase) which fails also on master).

I start now adding some features.

@QuentinN42
Copy link
Author

python -m yamllint tests/yaml-1.2-spec-examples/example-2.28 -f json

[
    {
        "line": 29,
        "column": 1,
        "desc": "too many blank lines (3 > 0)",
        "rule": "empty-lines",
        "level": "error",
        "file": "tests/yaml-1.2-spec-examples/example-2.28"
    }
]

@QuentinN42
Copy link
Author

python -m yamllint tests/yaml-1.2-spec-examples/example-7.10 -f junitxml

<?xml version="1.0" encoding="utf-8"?>
<testsuites>
    <testsuite name="pytest" errors="9" failures="1" skipped="0" tests="10" time="0" timestamp="2022-02-03T23:08:47.168125" hostname="E42">
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:2:1" name="document-start" time="0.0"><failure message="missing document start &quot;---&quot;"><\/failure><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:8:4" name="brackets" time="0.0"><error message="too many spaces inside brackets"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:8:4" name="colons" time="0.0"><error message="too many spaces before colon"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:8:5" name="None" time="0.0"><error message="syntax error: expected the node content, but found ':' (syntax)"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:8:6" name="indentation" time="0.0"><error message="cannot infer indentation: unexpected token"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:9:3" name="indentation" time="0.0"><error message="wrong indentation: expected 4 but found 2"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:10:3" name="indentation" time="0.0"><error message="wrong indentation: expected 4 but found 2"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:11:3" name="indentation" time="0.0"><error message="wrong indentation: expected 4 but found 2"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:12:3" name="indentation" time="0.0"><error message="wrong indentation: expected 4 but found 2"><\/error><\/testcase>
        <testcase classname="tests/yaml-1.2-spec-examples/example-7.10:12:29" name="brackets" time="0.0"><error message="too many spaces inside brackets"><\/error><\/testcase>
    </testsuite>
</testsuites>

@adrienverge
Copy link
Owner

adrienverge commented Feb 5, 2022

Hello,

See #441 (comment) (and other issues) for why junitxml format won't be accepted.

About JSON, there are already 2 open pull requests: #245 and #68, their authors abandoned them. Maybe it's better to start from there?

As described in previous discussions, the output would be better like:

{
  "path": ...,
  "line": ...,
  "column": ...,
  "message": ...,
  "rule": ...,
  "level": "warning|error"
}

Also I see this pull request doesn't have tests and introduces small unrelated changes, please check out CONTRIBUTING.rst. Thanks!

@QuentinN42
Copy link
Author

QuentinN42 commented Feb 5, 2022

As I can see the 'message' seems to be : [warning] missing starting space in comment (comments)
But it can be inferred from level, desc and rule...

I think it's better to keep all this fields separates if you want an output as JSON it's likely to be sent through an API or thx like that...

@QuentinN42
Copy link
Author

And yes tests will come this WE.

@QuentinN42
Copy link
Author

A lot of code modification comes form refactoring.

Trough this point there is only some refactors. All tests still pass (except test_run_with_user_global_config_file (tests.test_cli.CommandLineTestCase) which fails also on master).

I start now adding some features.

@QuentinN42
Copy link
Author

So the new code added are from this two commits :

@QuentinN42
Copy link
Author

If you want the "description" as formatted as #245, you can simply use jq :
python -m yamllint tests/yaml-1.2-spec-examples/example-2.28 -f json | jq '.[] | .message = ("["+.level+"] "+.desc+" ("+.rule+")")'

{
  "line": 29,
  "column": 1,
  "desc": "too many blank lines (3 > 0)",
  "rule": "empty-lines",
  "level": "error",
  "path": "tests/yaml-1.2-spec-examples/example-2.28",
  "message": "[error] too many blank lines (3 > 0) (empty-lines)"
}

Or I can add it to the current JSONFormater class, as you want.

@QuentinN42
Copy link
Author

The comment of @blui is valid (#245 (comment))...

I'll read the Codeclimate docs and then modify my output accordingly.

@QuentinN42
Copy link
Author

QuentinN42 commented Feb 5, 2022

Here is the format: GL docs, codeclimate docs.

@QuentinN42
Copy link
Author

python -m yamllint tests/yaml-1.2-spec-examples/example-2.28 -f codeclimate

[
    {
        "type": "issue",
        "check_name": "empty-lines",
        "description": "too many blank lines (3 > 0)",
        "content": "too many blank lines (3 > 0) (empty-lines)",
        "categories": [
            "Style"
        ],
        "location": {
            "path": "tests/yaml-1.2-spec-examples/example-2.28",
            "positions": {
                "begin": {
                    "line": 29,
                    "column": 1
                }
            }
        },
        "remediation_points": 1000,
        "severity": "major"
    }
]

@QuentinN42
Copy link
Author

When we are ok with the desired format @adrienverge I'll start writting some tests.

@adrienverge
Copy link
Owner

The desired format is:

{
  "path": "dir/file.yaml",
  "line": 1337,
  "column": 42,
  "message": "duplication of key \"k\" in mapping",
  "rule": "key-duplicates",
  "level": "error"  # or "warning"
}

Can you please post only one message, when the PR is fully ready?

@QuentinN42
Copy link
Author

Is it normal that the test test_run_with_user_global_config_file failed on master ?

❯ python -m unittest discover
======================================================================
FAIL: test_run_with_user_global_config_file (tests.test_cli.CommandLineTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/n42/git/yamllint/tests/test_cli.py", line 315, in test_run_with_user_global_config_file
    self.assertEqual(ctx.returncode, 0)
AssertionError: 1 != 0

----------------------------------------------------------------------
Ran 356 tests in 4.294s

FAILED (failures=1)

@QuentinN42
Copy link
Author

@adrienverge I think it's ready, I've added 100 test to reach 98 coverage %.

Comment on lines +308 to +310
# remove other env vars to make sure we are using the HOME config file.
os.environ.pop('YAMLLINT_CONFIG_FILE', None)
os.environ.pop('XDG_CONFIG_HOME', None)
Copy link
Author

Choose a reason for hiding this comment

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

I've also patched the test_run_with_user_global_config_file test by adding removing other env vars.

@@ -45,7 +45,7 @@ jobs:
python-version: ${{ matrix.python-version }}
- name: Append GitHub Actions system path
run: echo "$HOME/.local/bin" >> $GITHUB_PATH
- run: pip install coveralls
- run: pip install coveralls ddt
Copy link
Author

Choose a reason for hiding this comment

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

I've added this module to make pytest.mark.parametrized like tests.
I don't konw if there is a built in unittest solution for that.

Comment on lines +17 to +22
pip install coveralls ddt
# all tests...
python -m coverage run --source=yamllint -m unittest discover
coverage report
# or just some tests (faster)
python -m unittest tests/rules/test_commas.py
Copy link
Author

Choose a reason for hiding this comment

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

Updated the docs to show coverage when running tests.

column,
desc='<no description>',
rule=None,
level=None
Copy link
Author

Choose a reason for hiding this comment

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

level can be passed as an optional parameter to simplify uses after.

"desc": self.desc,
"rule": self.rule,
"level": self.level,
"message": f"[{self.level}] {self.message}"
Copy link
Author

Choose a reason for hiding this comment

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

Message field match the parsable output style.

Comment on lines +64 to +70
if name == 'auto':
if run_on_gh():
name = 'github'
elif supports_color():
name = 'colored'
else:
name = 'standard'
Copy link
Author

Choose a reason for hiding this comment

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

This is the uncovered code (with also run_on_gh & supports_color).

I don't know how to test it because it's platform dependent...

@KyleTryon
Copy link

Very hopeful to see JUNIT formatting 🙇

@QuentinN42
Copy link
Author

@adrienverge the pr is ready, when do you think you'll be available to review it?

@adrienverge
Copy link
Owner

@QuentinN42 no the PR is not ready: it's very messy, very verbose and I cannot count the number of unrelated changes it adds. Contributing guidelines are still not followed, and my previous requests are not addressed. Sorry for saying it rough, but I am a volunteer and cannot spend so much time reviewing this kind of work. Sorry again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants