-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[R] [ci] R linters are not being run in CI #5796
Comments
@hcho3 would you be open to me submitting a PR that does what I've described (moving linting out of the R unit tests into a standalone script)? I think it would make sense for me to do that and only include the linters that can impact code correctness (like |
@jameslamb Thanks! That would be great. You don't have to address linting error on the PR, we can follow up by pushing to your branch or open another PR. |
I opened a PR for fixing existing linting rules: #5911 |
oh nice! |
#5911 added lintr in CI. |
A better script is still needed. Currently I think on github action it shows only you have errors, but not line number. |
@trivialfis I actually noticed this too. I wonder if running lintr in Ubuntu would fix the issue. |
I think we need to call lint package explicitly. |
Sure, let me prepare a pull request. I'd like to see clear lintr warnings without having to run it manually. |
it seems that the R linters are not running in CI right now. This is from the most recent build on I wanted to try fixing some issues found by other linters tonight. Without adding any new linters, I ran this cd R-package
Rscript tests/run_lint.R A lot of linting issues were found, and |
@jameslamb It looks like GitHub Actions job timed out. I restarted the test. As for the issues, make sure to install XGBoost R package first by running This is from the previous commit of the master branch: https://github.com/dmlc/xgboost/runs/948386277?check_suite_focus=true, where the linter check passed. |
ohhhhh I see! I should have looked at the GitHub Actions config, I see that now. Thank you |
I tried to pick up part of #5779 tonight, and realized something...I believe that the R linting tests are not being run in CI.
I confirmed this by copying those linters used in
test_lint.R
into the following scriptlintr script (click me)
And then running it like this
Rscript lintr_r_code.R $(pwd)/R-package
I think the issue is that
test_lint.R
includes askip_on_cran()
xgboost/R-package/tests/testthat/test_lint.R
Line 3 in b47b5ac
And R tests are run by building a package and using
R CMD check --as-cran
xgboost/tests/ci_build/build_test_rpkg.sh
Line 22 in b47b5ac
xgboost/appveyor.yml
Line 106 in b47b5ac
When I run the script above, it finds 900+ linting issues.
First 100 R linter issues (click me)
Proposal for fixing this issue
I think it would be beneficial to move linting out of the R unit tests into a standalone script, like the approach we use in
LightGBM
https://github.com/microsoft/LightGBM/blob/4d43e96bbc10de2937aa70a5829977d46bdb1852/.ci/test.sh#L57
If you do that, all
skip_*
logic can be removed, and it will be really obvious if the tests are not passing.The text was updated successfully, but these errors were encountered: