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

Fix problems with r-lintr, add unit tests #4235

Merged
merged 11 commits into from
Nov 11, 2024

Conversation

nwiltsie
Copy link
Contributor

@nwiltsie nwiltsie commented Nov 9, 2024

Fixes #2885, fixes #2886.

I'm back to try again after I was stymied with #2887!

Proposed Changes

  1. Remove the purrr::keep filter for lints.
    • First, as-written the filter crashes if there are any lints (lintr crashes with any lints #2886). This hasn't been caught because only the "bad" files produce lints, and crashes aren't distinguished from clean error exits.
    • Second, theoretically lintr lints are typed as "style", "warning", and "error", but in practice I couldn't find any syntactically-valid code that raised errors. The existing "bad" test case only produces style and warning lints.
  2. Add additional test case files for lintr.
    • I added duplicates of the existing good and bad files in a subfolder, testing Issue with lintr under mega-linter-runner #2885.
    • I added another subfolder with a "good" file containing lints that are disabled by a .lintr file.
    • I also removed a bunch of stray comments from the existing "good" test case file.
  3. Remove TEMPLATE/.lintr file and update logic for supplying user value.

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@echoix
Copy link
Collaborator

echoix commented Nov 9, 2024

Sorry, it is not a real approval, I just mispressed on the phone

Copy link
Collaborator

@echoix echoix left a comment

Choose a reason for hiding this comment

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

I remember having to update the R tests, I think the test files (with the comments), were directly from the projects' test files.

The good files should be what the bad file should be once formatted by the tool. So I would be suprised that the comments removed only in the good file (that were there from the project test files) would work as expected. (Unless the tool strips comments). You can wait to see test failures to be sure.

@nwiltsie
Copy link
Contributor Author

nwiltsie commented Nov 9, 2024

The good files should be what the bad file should be once formatted by the tool. So I would be suprised that the comments removed only in the good file (that were there from the project test files) would work as expected.

It seems like the R tests passed just fine - 2/3 of the failed checks seem unrelated to these changes, and the third seems to be an over-aggressive spell-checker. I'm not sure how to proceed from here - do you have any advice?

(No rush, it's a long weekend here in the US and I'm idly checking this.)

Tests + Deploy Docker Image - DEV

Line 1309: [gw2] [ 63%] PASSED megalinter/tests/test_megalinter/linters/r_lintr_test.py::r_lintr_test::test_failure

Line 1337: [gw2] [ 64%] PASSED megalinter/tests/test_megalinter/linters/r_lintr_test.py::r_lintr_test::test_success

It seems like the errors were with the markdown tests?

=========================== short test summary info ============================
FAILED megalinter/tests/test_megalinter/linters/markdown_markdown_link_check_test.py::markdown_markdown_link_check_test::test_failure
FAILED megalinter/tests/test_megalinter/linters/markdown_markdown_link_check_test.py::markdown_markdown_link_check_test::test_success
= 2 failed, 604 passed, 328 skipped, 52 warnings, 6 rerun in 642.74s (0:10:42) =
Pytest exited 1
Error(s) found by Pytest

Gitpod / build:

These errors relate to installing uv?

------
Dockerfile:94
--------------------
  93 |     # hadolint ignore=SC1090,SC1091
  94 | >>> RUN curl  -LsSf https://astral.sh/uv/install.sh | sh \
  95 | >>>     && source "$HOME/.cargo/env"
  96 |     
--------------------
ERROR: failed to solve: process "/bin/bash -o pipefail -c curl  -LsSf https://astral.sh/uv/install.sh | sh     && source \"$HOME/.cargo/env\"" did not complete successfully: exit code: 1
make[1]: *** [.config/make/gitpod.mak:4: gitpod-build] Error 1
make: *** [.config/make/gitpod.mak:12: gitpod-tests] Error 2
Error: Final attempt failed. Child_process exited with error code 2

Megalinter

The spell-checker is complaining about the embedded R-code:

  megalinter/linters/RLinter.py:17:15     - Unknown word (setwd)      -- f"setwd('{Path(file).parent
  	 Suggestions: [stew, stead, steed, stews, stewed]

@nvuillam
Copy link
Member

nvuillam commented Nov 9, 2024

@nwiltsie thanks for your PR :)

You can update .cspell.json to add setwd in allowed words

The other issues are indeed not related to your PR, once we'll fix them in main you'll be able to merge the new main in your branch and your PR will pass :)

@nvuillam
Copy link
Member

@nwiltsie i updated your branch, let's see if it passes :)

@nvuillam nvuillam merged commit 5f8f82f into oxsecurity:main Nov 11, 2024
6 checks passed
@nwiltsie nwiltsie deleted the fix-lintr-errors branch November 11, 2024 19:02
@nwiltsie
Copy link
Contributor Author

Thanks @nvuillam!

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.

lintr crashes with any lints Issue with lintr under mega-linter-runner
3 participants