Refactoring of entry point script

[sthagen]

Just in case this is a welcome enhancement I refactored the entry point script to make it more extensible and maintainable.

No hard feelings if you ignore or directly reject the request.

The latter may be very subjective.

I find it tricky to refactor without tests, so I wrote a context module (to mock the non-forgiving environemental reads) and a test module which I place side by side in my working are inside the src folder:

$ tree src
src
├── context.py
├── entrypoint.py
└── test_entrypoint.py

I understood from the github actions documentation that the tests files might be too much in the repo which seeds the market place action, so I just put them - as all my contributions under license MIT - into this PR as comment:

context.py:

import os

os.environ["GITHUB_EVENT_NAME"] = "GITHUB_EVENT_NAME"
os.environ["GITHUB_REPOSITORY"] = "GITHUB_REPOSITORY"
os.environ["GITHUB_REF"] = "GITHUB_REF"
os.environ["GITHUB_HEAD_REF"] = "GITHUB_HEAD_REF"
os.environ["GITHUB_BASE_REF"] = "GITHUB_BASE_REF"
os.environ["GITHUB_ACTOR"] = "GITHUB_ACTOR"
os.environ["GITHUB_REPOSITORY_OWNER"] = "GITHUB_REPOSITORY_OWNER"
os.environ["INPUT_GITHUB_TOKEN"] = "INPUT_GITHUB_TOKEN"

test_entrypoint.py:

# pylint: disable=line-too-long,missing-function-docstring,missing-module-docstring
import context as ctx  # pylint: disable=unused-import
import entrypoint as ep

EMPTY_STR = ''
PLAUSIBLE_CHECKS = ep.KNOWN_CHECKS

def test_split_csv_empty_ok():
    assert next(ep.split_csv(EMPTY_STR)) == EMPTY_STR

def test_split_csv_single_ok():
    assert next(ep.split_csv('no_separator_in_here')) == 'no_separator_in_here'

def test_split_csv_plausible_ok():
    for ndx, check in enumerate(ep.split_csv(ep.CHECKS_SEP.join(PLAUSIBLE_CHECKS))):
        assert check == PLAUSIBLE_CHECKS[ndx]

def test_split_csv_sep_at_begin_ok():
    assert next(ep.split_csv(f'{ep.CHECKS_SEP}separator_at_begin')) == EMPTY_STR

def test_split_csv_sep_in_middle_ok():
    assert next(ep.split_csv(f'separator{ep.CHECKS_SEP}between')) == 'separator'

def test_split_csv_sep_at_end_ok():
    assert next(ep.split_csv(f'separator_at_end{ep.CHECKS_SEP}')) == 'separator_at_end'

def test_is_valid_empty_ok():
    assert ep.is_valid(EMPTY_STR) == EMPTY_STR

def test_is_valid_known_ok():
    for check in PLAUSIBLE_CHECKS:
        assert ep.is_valid(check) == check

def test_parse_scopes_empty_ok():
    dsl = {ep.ENABLE_CHECKS: EMPTY_STR}
    assert ep.parse_checks(dsl) == []

def test_parse_scopes_every_ok():
    dsl = {ep.ENABLE_CHECKS: ep.CHECKS_SEP.join(PLAUSIBLE_CHECKS)}
    assert ep.parse_checks(dsl) == [ep.CHECK_EVERYTHING]

def test_parse_scopes_some_not_all_ok():
    selection = [ch for ch in PLAUSIBLE_CHECKS[:len(PLAUSIBLE_CHECKS) // 2] if ch != ep.CHECK_EVERYTHING]
    dsl = {ep.ENABLE_CHECKS: ep.CHECKS_SEP.join(selection)}
    assert ep.parse_checks(dsl) == selection

def test_command_empty_ok():
    assert ep.command() == ['cppcheck', f'--enable={ep.CHECK_EVERYTHING}', '--inconclusive']

def test_main_empty_ok():
    ep.GITHUB_EVENT_NAME = "pull_request"
    assert ep.main() is None

def test_main_plausible_ok(capsys):
    ep.GITHUB_EVENT_NAME = "pull_request"
    ep.GITHUB_ACTOR = ep.GITHUB_REPOSITORY_OWNER
    assert ep.main() is None
    out_expected = (
        'given command cppcheck --enable=all --inconclusive '
        '--output-file=cppcheck_report.txt .\n'
        'checking version'
    )
    out, err = capsys.readouterr()
    assert out.strip() == out_expected.strip()
    assert err.strip() == EMPTY_STR

Executing these tests within a python 3.8 virtual environment with pytest installed gives:

$ pytest -q
..............                                                                                                                                   [100%]
14 passed in 0.08s

And coverage seems to be OK ...

$ pytest --cov=src --cov-report=term -q
..............                                                                                                                                   [100%]

---------- coverage: platform darwin, python 3.8.0-final-0 -----------
Name                     Stmts   Miss  Cover
--------------------------------------------
src/context.py               9      0   100%
src/entrypoint.py           77      0   100%
src/test_entrypoint.py      45      0   100%
--------------------------------------------
TOTAL                      131      0   100%

14 passed in 0.30s

My linter only notices the original TODO:

$ pylint --output-format=json --reports=y src/entrypoint.py 

Output:

[
    {
        "type": "warning",
        "module": "entrypoint",
        "obj": "",
        "line": 20,
        "column": 2,
        "path": "src/entrypoint.py",
        "symbol": "fixme",
        "message": "TODO: How about PRs from forks?",
        "message-id": "W0511"
    }
]

Also, mypy thinks the types are OK (but then there are no annotations).

[deep5050]

first of all, hi @sthagen , huge thanks for the interest 😍. thanks for the effort , i was planning for a long time to refactor this but could not manage time to do so.

let me ask you something. Did the test pass? i cannot see any action log on your repo. As you have changed a lot of things, let me go through the code first.

I'll get back to you soon :).

also i need a maintainer for this repo ,let me know if you are interested :)

[deep5050]

Hi, @sthagen I just checked your code. Great work. ❤️ , I'm impressed

However, It has some issues to solve.

for some reason the main command for cppcheck along with its options, not executing. I guess only cppcheck is given as a command and the whole options are not getting added. That's why it is failing to generate the report. Please look at the vector array. If you find the issue let me know, else I'll take some time to look into it :+1:

I also encourage you to enable action on your repo and test it there.

there should be something like this in the log.

1/3 files checked 34% done
Checking __tests__/server/server.c ...
2/3 files checked 64% done
Checking __tests__/test.c ...
Checking __tests__/test.c: PROCESS...
Checking __tests__/test.c: THREAD...
3/3 files checked 100% done

please update to skip_preprocessor: disable (in the YAML file) while testing

[sthagen]

I did activate the test action as suggested by @deep5050, amended the PR branch with a commit that passed local tests and exploratory testing, and saw the github test action succeed in the cppcheck step (but fail in the publish for a merge commit I had to perform to include the action activation changes).

Hope this helps.

[deep5050]

publish action is not directly connected with my action. It's failing because your main branch is way behind your cleaning-person branch. that's not an issue.

glad to see cppcheck passed :cool: let me test this in a separate branch and then I will merge it into my main branch.

[deep5050]

Let me ask you something, would you please take some time to add a little feature to it once I merge this?

Let users specify custom options ( without checking them inside my code). As many options are added to the cppcheck tool on every new release, It's not possible to cover all of them. so I was planning to add an environment variable like other_options where users can add options on their own.

[deep5050]

Hi @sthagen, I must say you are an excellent programmer. If you are interested let's build something together. Let's list our ideas and see if there is anything we can work by collaborating. Also, I will be grateful to you if you take care of this repo along with me in the future. :heart:

[baderouaich]

Hello @sthagen, thank you for the additions! i had an issue with option exclude_check, it seems to be ignored. i guess it has to do with the -i {{}} in entrypoint.py Line 99 ?

here is my .yml config: https://github.com/BaderEddineOuaich/Enigma/blob/master/.github/workflows/static-analysis.yml here is the latest action done: https://github.com/BaderEddineOuaich/Enigma/runs/1617993682 and thats the generated file: https://github.com/BaderEddineOuaich/Enigma/blob/master/CppCheck-Static-Analysis-Report.txt where ./Dependencies/ should be ignored. i hope that will help you solve the issue. thanks!

[sthagen]

... i had an issue with option exclude_check, it seems to be ignored. i guess it has to do with the -i {{}} in entrypoint.py Line 99 ? ...

Yes. Thanks a lot for spotting. I sneaked the fix via an amended commit into PR #23

@deep5050

[deep5050]

@sthagen #23 is still under review, but the issue mentioned by @BaderEddineOuaich needs to be solved ASAP as it is in the main branch.

I'm fixing it directly on my main branch.

[deep5050]

@BaderEddineOuaich rerun the action and let me know if that worked. i have modified the code.

[baderouaich]

@deep5050 the issue still persists as ./Dependencies/ is still being analyzed in the latest run

output.txt

[sthagen]

Ah, an upstream problem. Thank you for providing the links to your assets.

@deep5050 the issue still persists as ./Dependencies/ is still being analyzed in the latest run

output.txt

I fixed the problem by applying the single-space fix that cures commandline parsers since decades ... :wink:

The version 1.9 of cppcheck does accept -i source_root_to_be_ignored while the newer one we are using does not.

When I remove the separating space between the -i and the folder name the new version of cppcheck also accepts it.

I will provide a hotfix from the main branch ... and created the PR #25 - sorry for the inconvenience.

@deep5050: Where should I place my tests so we can avoid future regressions like these? I do not know much about constraints on github-actions source repositories, but I think I read only minimal files for deployment should be in there, but then I may very well be wrong (hopefully).

Thanks.

[baderouaich]

it works like a charm now. thank you guys for the effort!