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

covr fails because testthat fails because lintr fails because it ignores .lintr #253

Closed
mschilli87 opened this issue Mar 15, 2017 · 6 comments

Comments

@mschilli87
Copy link

Hi @jimhester,

I am not 100% if this is a covr or lintr issue but I have the following problem:

I run testthat test including linting with lintr to ensure proper style.
There are a few linters I don't want to run (as I break their style suggestions willingly).
Those, I disable through a .lintr file.
Without covr this works just fine (thus this covr issue).

If, for the same repository, I add covr (without linting), it works, too (thus maybe a lintr issue)?

However, if I enable both covr & lintr, testthat tests run successfully (incl. linting, but ignoring the linters specified in .lintr), but covr fails and @lintr-bot complains about the disabled linters.

I tracked this down to coveralls() running the tests (incl. linting) which fail, as lintr complains about breaking the disabled linters.

I'm working on a minimal working example to better demonstrate the issue, but maybe this is already enough to help me.

Best,
Marcel

@mschilli87
Copy link
Author

mschilli87 commented Mar 15, 2017

I was able to reproduce this issue in a minimal package:

  1. I creteated a buildable package with a single function using camelCase (negative control) and = assignment (positive control)
  2. I setup Travis CI incl. testthat to make sure everything works as expected.
  3. I branched off and added lintr detecting both style issues.
  4. I setup .lintr to ignore the camelCase (as it's cool and camel_case_linter is as oxymoronic as it gets anyways 😉).
  5. I fixed the = assignment in a feature branch
  6. I added covr in a new branch based on the style fixed branch.
  7. I tried to merge the covr branch into the lintr one

I did my best to make this as simple, self-contained and comprehensible as possible:
https://github.com/mschilli87/testCovrVsLintr/network


The issue gets clear by comparing mschilli87/testCovrVsLintr#1 and mschilli87/testCovrVsLintr#2:

  • Without covr, all test pass.
  • With covr, the tests fail and @lintr-bot complains about the camelCase again.

The only difference betwen those two pull requests is mschilli87/testCovrVsLintr@faa5be7.


edit: Correction: The tests actually pass in mschilli87/testCovrVsLintr#2 (when run by R CMD check) but the codecov report is never sent as coveralls() dies with the failing test through lintr's complaining.

@jimhester
Copy link
Member

The issue is the .lintr is not included in your built package, so the tests do not find it. You can try putting the .lintr file in inst/, which should include it in your built package.

@mschilli87
Copy link
Author

Thanks a lot @jimhester!

  1. I tried moving .lintr to inst/.lintr but this (expectedly[?]) broke R CMD check (see mschilli87/testCovrVsLintr@770f4a6)
  2. I then tried copying .lintr to inst/.lintr and merge in covr and finally I've got lintr to pass during R CMD check and covr to run without triggering @lintr-bot (see combining covr with lintr with copied .lintr mschilli87/testCovrVsLintr#3).

I still see two issues with this solution:

  1. It is not documented anywhere (that I could find).
  2. It requires maintaining two identical copies of the lintr configuration file.

The first point could well be my mistake in searching for a solution and otherwise would probably better be addresses separately (as a lintr issue).

The second point however IMHO creates the potential for further problems (when someone updates .lintr without being aware of ints/.lintr).
I thought about symlinking inst/.lintr to .lintr to overcome this but as I want my R packages to build platform independently, I doubt that this would work out.
Of course, this could be addressed through git hooks or other tricks work-arounds but nothing really clean and satisfactory comes to mind right now (at least to me).

Best,
Marcel

@BenoitLondon
Copy link

Hi all,

I had the same issue and kept searching for a solution :)

I think a good solution would be for lintr package to search in the root folder and in the inst folder for a .lintr file ?

Meanwhile I went with the two files solution...

cheers,
Benoit

@jimhester
Copy link
Member

Symlinks work fine cross platform, just symlink to the file in inst/

@mschilli87
Copy link
Author

In this case, all that's left for me to complain about is that either the symlinking should be recommended in the README or (even better) @BenoitLondon's suggestion could be implemented.

@jimhester: If you are fine with allowing just inst/.lintr but you don't have time to implement it, I'd be willing to prepare a PR.

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

No branches or pull requests

3 participants