Format fix tests #2294

bdovaz · 2023-01-26T22:36:14Z

Resolves #2250

nvuillam · 2023-01-26T22:39:17Z

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ BASH	bash-exec	6		0	0.01s
✅ BASH	shellcheck	6		0	0.15s
✅ BASH	shfmt	6	0	0	0.44s
✅ COPYPASTE	jscpd	yes		no	3.62s
✅ DOCKERFILE	hadolint	114		0	21.19s
✅ JSON	eslint-plugin-jsonc	21	0	0	2.59s
✅ JSON	jsonlint	19		0	0.37s
✅ JSON	v8r	21		0	14.68s
⚠️ MARKDOWN	markdownlint	309	0	230	7.33s
✅ MARKDOWN	markdown-link-check	309		0	6.15s
✅ MARKDOWN	markdown-table-formatter	309	0	0	20.97s
✅ OPENAPI	spectral	1		0	2.29s
⚠️ PYTHON	bandit	183		47	2.63s
✅ PYTHON	black	183	0	0	5.09s
✅ PYTHON	flake8	183		0	2.23s
✅ PYTHON	isort	183	0	0	0.94s
✅ PYTHON	mypy	183		0	9.36s
✅ PYTHON	pylint	183		0	14.79s
⚠️ PYTHON	pyright	183		246	22.5s
✅ REPOSITORY	checkov	yes		no	36.05s
✅ REPOSITORY	git_diff	yes		no	0.44s
✅ REPOSITORY	secretlint	yes		no	16.59s
✅ REPOSITORY	trivy	yes		no	35.63s
✅ SPELL	cspell	745		0	26.53s
✅ SPELL	misspell	566	0	0	1.15s
✅ XML	xmllint	3	0	0	0.43s
✅ YAML	prettier	81	0	0	3.68s
✅ YAML	v8r	23		0	71.79s
✅ YAML	yamllint	82		0	1.28s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

bdovaz · 2023-01-26T22:45:31Z

@nvuillam the architecture is more or less solved, now it is a matter of adding the test files for each linter that allows format/fix and see how the tests are passing.

I have found special cases in which I have had to pass certain variables to make them work as you can see.

megalinter/tests/test_megalinter/LinterTestRoot.py

nvuillam · 2023-01-28T22:07:25Z

@bdovaz that looks very promising :)

bdovaz · 2023-01-28T23:11:16Z

@nvuillam is curious because with this PR when running the tests in different linters I am finding several bugs in several linters that did not run correctly the format or autofix ....

I guess it's just a coincidence that no one has tested the format/autofix on these linters so far and that's why we haven't noticed it.

So we kill two birds with one stone!

nvuillam · 2023-01-29T10:33:48Z

@bdovaz indeed, many autoformat-fix may not have been tested... your PR will clean some mess ! :)

bdovaz · 2023-01-29T13:33:36Z

@nvuillam you may know more than me about this subject.... The tests that I am preparing (creating the input files + settings if necessary) run correctly locally but not in github actions.

They all fail for the same reason. If you look at the error is this:

It originates here:

https://github.com/oxsecurity/megalinter/blob/dev/format-fix-tests/megalinter/tests/test_megalinter/helpers/utilstest.py#L679

And I set the value here which I'm afraid is what varies with respect to my machine locally? @nvuillam the key is how to set the git repository root correctly for all cases so that the git APIs work correctly. Can you help me?

https://github.com/oxsecurity/megalinter/blob/dev/format-fix-tests/megalinter/tests/test_megalinter/LinterTestRoot.py#L27

I would appreciate your help to unlock this problem, thanks!

echoix · 2023-01-29T15:29:13Z

When the tests are run (where it fails), is it inside a container image? If I recall correctly, there is a step when building that uses the previously built image and tries to run tests. And if it's that case, I think we don't place the repo inside the container, only the code. So not git there. Does the tests need to have a repo? Can megalinter run on a local folder without having it inside a git repo?

bdovaz · 2023-01-29T15:31:18Z

When the tests are run (where it fails), is it inside a container image? If I recall correctly, there is a step when building that uses the previously built image and tries to run tests. And if it's that case, I think we don't place the repo inside the container, only the code. So not git there. Does the tests need to have a repo? Can megalinter run on a local folder without having it inside a git repo?

@echoix the need for the repo comes from a message from @nvuillam on how he wanted it implemented:

#2250 (comment)

nvuillam · 2023-01-29T16:39:41Z

There are different ways to create Repo object in existing sources... maybe you can try those ?

ex: repo = git.Repo(os.path.realpath(self.github_workspace))

bdovaz · 2023-01-29T16:44:43Z

@nvuillam did you read @echoix's answer? Being what he says, your solution would not work either.

What I have done is to handle the error and assume that in local it will work and in the container it will not.

echoix · 2023-01-29T16:46:53Z

My answer was conditional. Is that code run as the Python code of megalinter inside the container? Or it is mapped into a folder outside the container, where a repo could exist?

bdovaz · 2023-01-29T16:50:30Z

My answer was conditional. Is that code run as the Python code of megalinter inside the container? Or it is mapped into a folder outside the container, where a repo could exist?

@echoix On my local machine I run it from VS Code obviously running it on my own machine without images or Docker containers, this works perfectly.

But when I do push and it runs in Github actions it runs from a container and in that case as you say there is no .git folder and that is why the code fails.

That is why I had to do this:

https://github.com/oxsecurity/megalinter/blob/dev/format-fix-tests/megalinter/tests/test_megalinter/helpers/utilstest.py#L681

What I want to know is, have I done well? Is there any other alternative @nvuillam?

nvuillam · 2023-01-29T16:50:43Z

On the single existing fixes test , there is a call to git so it works at least in CI

megalinter/megalinter/tests/test_megalinter/mega_linter_2_fixes_test.py

Line 76 in 40631ba

for fixable_file in fixable_files:

megalinter/megalinter/tests/test_megalinter/helpers/utilstest.py

Line 595 in 40631ba

def assert_file_has_been_updated(file_name, bool_val, test_self):

I don't remember locally without container... but we have megalinter repo when we run test cases in megalinter

bdovaz · 2023-01-29T17:11:10Z

On the single existing fixes test , there is a call to git so it works at least in CI

megalinter/megalinter/tests/test_megalinter/mega_linter_2_fixes_test.py

Line 76 in 40631ba

for fixable_file in fixable_files:

megalinter/megalinter/tests/test_megalinter/helpers/utilstest.py

Line 595 in 40631ba

def assert_file_has_been_updated(file_name, bool_val, test_self):

I don't remember locally without container... but we have megalinter repo when we run test cases in megalinter

Ok, I'll try that, let's see.

And another thing @nvuillam, the list of linters of this workflow why doesn't have all of them? Because you have to keep it by hand and it is not synchronized and you need to modify it? Or that fixed list is like that for something? Because right now my problem is that not having all of them, I will not be able to test all the linters in this PR:

megalinter/.github/workflows/deploy-DEV-linters.yml

Line 56 in 40631ba

linter:

codecov-commenter · 2023-01-29T17:22:03Z

Codecov Report

Merging #2294 (40631ba) into main (f89461a) will increase coverage by 0.12%.
The diff coverage is 20.00%.

❗ Current head 40631ba differs from pull request most recent head 35a6353. Consider uploading reports for the commit 35a6353 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2294      +/-   ##
==========================================
+ Coverage   82.39%   82.51%   +0.12%     
==========================================
  Files         171      171              
  Lines        4521     4513       -8     
==========================================
- Hits         3725     3724       -1     
+ Misses        796      789       -7

Impacted Files	Coverage Δ
megalinter/flavor_factory.py	`77.46% <20.00%> (+6.57%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

echoix · 2023-01-29T18:09:12Z

@bdovaz

And another thing @nvuillam, the list of linters of this workflow why doesn't have all of them? Because you have to keep it by hand and it is not synchronized and you need to modify it? Or that fixed list is like that for something? Because right now my problem is that not having all of them, I will not be able to test all the linters in this PR:

https://github.com/oxsecurity/megalinter/blob/main/.github/workflows/deploy-DEV-linters.yml#L56

Isn't that the linters that can output sarif, and that are packaged individually?

Also maybe it could be helpful when you post links in comments to use the permalinks, so that if someone else comes back and read, it'll still match the good lines even if the contents were to be updated (or the files removed). It's not much, but it makes sure that we keep talking about the same thing, and that we are both sure that it's the same thing.

nvuillam · 2023-01-29T18:31:08Z

Isn't that the linters that can output sarif, and that are packaged individually?

That's right :)

Ox.security service use them :)
Others do not handle SARIF so no need to release them individually

bdovaz · 2023-01-29T18:33:31Z

@nvuillam can you answer to this comment please?

#2294 (comment)

nvuillam · 2023-01-29T18:37:26Z

@bdovaz all linters are tested in the main check job :)

bdovaz · 2023-01-29T18:44:35Z

@bdovaz all linters are tested in the main check job :)

@nvuillam by main check job do you mean this one?

https://github.com/oxsecurity/megalinter/actions/runs/4038035333/jobs/6941752465

It is the one that takes the longest to run and it does not run in parallel each linter like the rest.

Still you didn't answer my question, in that workflow we can/should add the remaining linters? For me at least it would help me to test everything much faster.

nvuillam · 2023-01-29T18:46:24Z

Yes it's the longest one, I know :/ But it tests all of them

As we are already struggling with github API limits that makes jobs fail, I prefer that we don't add individual linter job if they are not deployed in their own docker image

bdovaz · 2023-01-29T18:50:26Z

@nvuillam but according to this page it is 1,000 calls per hour, so many workflows (each one with its calls) are executed per hour to reach 1,000?

https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limits-for-requests-from-github-actions

nvuillam · 2023-01-29T18:55:16Z

When using GITHUB_TOKEN, the rate limit is 1,000 requests per hour per repository

The calls from Dockerfile do not use GITHUB_TOKEN , so we have way less :/ (don't know how many)

bdovaz · 2023-01-29T19:03:02Z

When using GITHUB_TOKEN, the rate limit is 1,000 requests per hour per repository

The calls from Dockerfile do not use GITHUB_TOKEN , so we have way less :/ (don't know how many)

@nvuillam But all this is not improvable? Can't we analyze and see if we can refactor something (I speak from ignorance) to avoid this limit?

nvuillam · 2023-01-29T19:04:59Z

@bdovaz there is... but it takes time ^^

#2256

bdovaz · 2023-01-29T23:28:54Z

@nvuillam I've encountered a problem with the linter you created (npm-groovy-lint)

https://github.com/oxsecurity/megalinter/actions/runs/4039087465/jobs/6943650830

Looking at the log, I think the problem is in the cli that returns exit code 1 even if the linter has correctly corrected the file, no?

echoix · 2023-02-13T23:43:43Z

For many packages i had to declare in the setup that they can be pushed.
But i'm not sure we really need to push them... can you try to set push: false and load: true in dev build linters yaml job descriptor ?

Until Docker and buildx changes (if it didn't solve the 2-3 years old problem in the last three weeks), the push: true will be needed when building multi platform images since load can't "load" images for another platform locally.

echoix · 2023-02-13T23:44:33Z

@nvuillam do you know what might be going on? I have a lot of these errors.

Is the registry logged in? And does the workflow has write permissions to GitHub packages? Maybe the answer is in the second idea

Kurt-von-Laven · 2023-02-13T23:57:23Z

I believe the syntax for testing @echoix's second thought would be:

permissions:
  packages: write

based on https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs.

echoix · 2023-02-14T00:45:32Z

In the page, after this,

Available scopes and access values:

permissions:
  actions: read|write|none
  checks: read|write|none
  contents: read|write|none
  deployments: read|write|none
  id-token: read|write|none
  issues: read|write|none
  discussions: read|write|none
  packages: read|write|none
  pages: read|write|none
  pull-requests: read|write|none
  repository-projects: read|write|none
  security-events: read|write|none
  statuses: read|write|none

If you specify the access for any of these scopes, all of those that are not specified are set to none.

So it's to keep in mind

bdovaz · 2023-02-14T22:39:07Z

@nvuillam is all set to be merge ready! What do you think?

nvuillam

Let's go :)
I'll have a look tomorrow (or before we geberate a new version ^^)

bdovaz · 2023-02-14T22:46:57Z

Great achievement to merge this branch.

If you look at the changelog, I have introduced many improvements / bug fixes.

nvuillam · 2023-02-14T22:47:19Z

Probably if we find a way to parallelize tests we'll gain some future PRs time :)

nvuillam · 2023-02-15T13:30:03Z

It broke something in auto-update linter jobs :/

bdovaz · 2023-02-15T16:14:13Z

@nvuillam I don't know what to fix because I don't even know what that workflow does but what I can tell you is that my PR fixes/changes the following in build.py:

Deletes all Dockerfile of individual linters before regenerating them to avoid "orphan" files, i.e. linters that no longer exist or have moved from "group"
Generate all individual linters, before my changes it generated part of them, not all of them
I apply the same logic to the generation of the test classes of the linters: delete all files before and regenerate them afterwards

That is, now new, modified or deleted files can appear in git. Before only modified because no files were deleted before generating them and not all of them were generated, only part of them.

I don't know if this gives you any clue to relate it to that problem and know how to solve it.

I have also seen in the log other errors:

https://github.com/oxsecurity/megalinter/actions/runs/4178767375/jobs/7237949515

nvuillam · 2023-02-15T18:19:33Z

I'll try to arrange that :)

bdovaz force-pushed the dev/format-fix-tests branch from a09a37f to 7f1994d Compare January 28, 2023 11:29

nvuillam reviewed Jan 28, 2023

View reviewed changes

megalinter/tests/test_megalinter/LinterTestRoot.py Outdated Show resolved Hide resolved

bdovaz mentioned this pull request Jan 29, 2023

Multi platform images #2273

Draft

bdovaz mentioned this pull request Jan 29, 2023

Docker build: Call Github API with authentication #2256

Closed

bdovaz and others added 9 commits February 13, 2023 22:56

Run build.sh --doc

a6a08f8

[MegaLinter] Apply linters fixes

cd596b3

Try to fix test

0528784

[MegaLinter] Apply linters fixes

7831575

Decouple pre_test from core and move to linters

f279421

Fix linter errors

025dcb8

[MegaLinter] Apply linters fixes

817a447

Update Dockerfiles

d9f82f1

Rename input test files

3c1a064

bdovaz force-pushed the dev/format-fix-tests branch from 32daedf to 3c1a064 Compare February 13, 2023 21:56

Fix tests

7f338af

bdovaz enabled auto-merge (squash) February 14, 2023 22:39

nvuillam approved these changes Feb 14, 2023

View reviewed changes

bdovaz merged commit 3c8b606 into main Feb 14, 2023

bdovaz deleted the dev/format-fix-tests branch February 14, 2023 22:46

bdovaz mentioned this pull request Feb 19, 2023

Doubling CI jobs issue for internal contributors #2377

Closed

Format fix tests #2294

Format fix tests #2294

Conversation

bdovaz commented Jan 26, 2023

nvuillam commented Jan 26, 2023 • edited Loading

🦙 MegaLinter status: ⚠️ WARNING

bdovaz commented Jan 26, 2023

nvuillam commented Jan 28, 2023

bdovaz commented Jan 28, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023 • edited Loading

echoix commented Jan 29, 2023

bdovaz commented Jan 29, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023 • edited Loading

echoix commented Jan 29, 2023

bdovaz commented Jan 29, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023 • edited Loading

codecov-commenter commented Jan 29, 2023

Codecov Report

echoix commented Jan 29, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023

nvuillam commented Jan 29, 2023

bdovaz commented Jan 29, 2023

echoix commented Feb 13, 2023

echoix commented Feb 13, 2023 • edited Loading

Kurt-von-Laven commented Feb 13, 2023 • edited Loading

echoix commented Feb 14, 2023 • edited Loading

bdovaz commented Feb 14, 2023

nvuillam left a comment

Choose a reason for hiding this comment

bdovaz commented Feb 14, 2023

nvuillam commented Feb 14, 2023

nvuillam commented Feb 15, 2023

bdovaz commented Feb 15, 2023

nvuillam commented Feb 15, 2023

nvuillam commented Jan 26, 2023 •

edited

Loading

bdovaz commented Jan 29, 2023 •

edited

Loading

bdovaz commented Jan 29, 2023 •

edited

Loading

bdovaz commented Jan 29, 2023 •

edited

Loading

echoix commented Feb 13, 2023 •

edited

Loading

Kurt-von-Laven commented Feb 13, 2023 •

edited

Loading

echoix commented Feb 14, 2023 •

edited

Loading