Fixes in tests + Coverage

[Melkiades]

I do not know the current coverage, also because I work from a server so I cannot check any word snapshot (which is the defualt for CI/CD, I think). On my machine the coverage is currently 60% with 400 missed lines for as_flextable. It should be doable. Beforehand, I moved out all library calls that should not be in tests if are between Imports: in the DESCRIPTION file. They should be anyway referenced :: if not in the test_that() window. For now, I am only checking if these work on the PR ci. I got many removals of snapshots and I do not know if it is expected (probably missing some snap-related libs due to the servers). So questions:

1. Should I reorder the DESCRIPTION file libs list (they should be in separate lines and alphabetic order)
2. Should I go through the tests and collect common data/fnc creations to add to the setup.R file?
3. In general, Imports: are not loaded directly in the testing, should I keep it like this? Note that if you need to do tests manually, devtools::load_all() does load the imports from package in the working directory.

[davidgohel]

Hello,

Should I reorder the DESCRIPTION file libs list (they should be in separate lines and alphabetic order)

I am sure it has not to be in separate lines to be valid DESCRIPTION but I don't mind if you write them in separate lines and alphabetic order. If you think it is easier to read, no problem at all

Should I go through the tests and collect common data/fnc creations to add to the setup.R file?

Why not deleting all visual tests made with doconv. I can isolate them on my disk and re-introduce them later. As I said before, I am not fully satisfied and I can see it generate confusion to collaborators. That way, you are free to work without the burden of these 4 files.

We can proceed as follows: we merge this PR, I remove the visual tests from the package and clean up what needs to be cleaned up. You can then resume from the new sources. I could re-inject them and clean up later.

What do you think about this?

In general, Imports: are not loaded directly in the testing, should I keep it like this? Note that if you need to do tests manually, devtools::load_all() does load the imports from package in the working directory.

I liked my library statements :) It helps me to work interactively.
But no problem if you want to delete them.

This were my thought: I need them to test, they are in the Imports/Suggests of the package (if 'suggests', it needs a testthat::skip_if_not_installed()). We can then attach the library. Using :: still add the necessary packages in the R environment even if not attached and I don't see the difference.

I am not clear, let me know :)

Best, David

[Melkiades]

Hello,

Should I reorder the DESCRIPTION file libs list (they should be in separate lines and alphabetic order)

I am sure it has not to be in separate lines to be valid DESCRIPTION but I don't mind if you write them in separate lines and alphabetic order. If you think it is easier to read, no problem at all

I think it is easier to search for libraries; I will do it, and if you do not like it we can revert ;)

Should I go through the tests and collect common data/fnc creations to add to the setup.R file?

Why not deleting all visual tests made with doconv. I can isolate them on my disk and re-introduce them later. As I said before, I am not fully satisfied and I can see it generate confusion to collaborators. That way, you are free to work without the burden of these 4 files.

We can proceed as follows: we merge this PR, I remove the visual tests from the package and clean up what needs to be cleaned up. You can then resume from the new sources. I could re-inject them and clean up later.

What do you think about this?

I have an idea. We could move the file in the snapshot folders from inst/ in the case it is installed for the manual testing. So we can still keep it in and have anyway a clean devtools::test output.

In general, Imports: are not loaded directly in the testing, should I keep it like this? Note that if you need to do tests manually, devtools::load_all() does load the imports from package in the working directory.

I liked my library statements :) It helps me to work interactively. But no problem if you want to delete them.

This were my thought: I need them to test, they are in the Imports/Suggests of the package (if 'suggests', it needs a testthat::skip_if_not_installed()). We can then attach the library. Using :: still add the necessary packages in the R environment even if not attached and I don't see the difference.

I understand very well, and adding the library calls when writing tests is natural. There are not major issues in keeping them if not for having clearer tests that differentiate what is in imports and suggests. In general, library calls should be in setup.R only nonetheless. I do not know if it is true, but I expect that repeated calls to library() might cause overhead, and it is usually the least maintained part of the code (e.g. if I do not use the lib anymore, it is easy to forget the library call there). For me, it helps also to know exactly which namespace I am referring to when I limit package library calls. What I generally learned to do is that in every session I import the libraries with devtools::load_all and rely on testing each file individually (or together) with devtools which takes into account this difference.

I hope to have convinced you! ^^ let me know what you think

[davidgohel]

Do as you wish, I'm already happy that the package has benefited from this attention and your time, I don't want to slow you down. We can always change things slightly later.

I just never thought about using devtools::load_all() for testing. I am convinced :)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 64.62%. Comparing base (ae707a0) to head (04cf35a).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #615 +/- ## ========================================== + Coverage 61.92% 64.62% +2.69% ========================================== Files 50 50 Lines 8604 8604 ========================================== + Hits 5328 5560 +232 + Misses 3276 3044 -232 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[davidgohel]

Hi @Melkiades, should I merge now or should I wait the removal of visual tests (snaps and involved tests)? I can also iterate over your branch if you need to. Let me know :)

[Melkiades]

Hi @Melkiades, should I merge now or should I wait the removal of visual tests (snaps and involved tests)? I can also iterate over your branch if you need to. Let me know :)

So in this PR, I moved the snapshots into inst and kept the tests with a flag in setup.R to activate these tests. On ci and in general I would keep this off. The only problem of this way is that once the files are copied and tested, you need to move them in inst manually and remove them. I thought about a smarter way to do this but within the test folder I would need {withr} in suggests/imports. I would wait to merge this PR until we get the coverage >80%. If you are definitive about taking out these tests from git or you have another solution, please do not hesitate to iterate on this PR! :)

[davidgohel]

Hi @Melkiades my week will be quite busy, but I did not forget this PR. I will try to achieve it during one evening of this week or next week

[Melkiades]

Hi @Melkiades my week will be quite busy, but I did not forget this PR. I will try to achieve it during one evening of this week or next week

Thank you David! I think I found a solution over the weekend for the snapshots, I will complete this PR today, and then when you have time you can review it and add/change what you want or need ;)

[Melkiades]

@davidgohel we might want to merge and then up the coverage in another PR. Here, we fix more the testing of snapshots, that now can be done on demand. I could not fully test it as I do not have MSOffice on the server I use, so if you have time to check that all paths and withr calls work, that would be great! :)

[davidgohel]

@Melkiades Thanks, everything works. I'll have to make a minor correction for the visual tests.

I'm merging so we can move forward