Fixes in tests + Coverage #615

Melkiades · 2024-03-13T10:36:05Z

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:

Should I reorder the DESCRIPTION file libs list (they should be in separate lines and alphabetic order)
Should I go through the tests and collect common data/fnc creations to add to the setup.R file?
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.

Melkiades · 2024-03-13T10:36:57Z

tests/testthat/setup.R

@@ -1,3 +1,5 @@
+library(xml2) # This should be imported already


Being outside the test_that() calls it needs to have :: or imported with library()

tests/testthat/test-borders.R

Melkiades · 2024-03-13T10:38:27Z

tests/testthat/test-borders.R

+  #   output_file = pdf_file,
+  #   envir = new.env(),
+  #   quiet = TRUE
+  # ) # need to tinytex::install_tinytex()


this is my local's problem, still to fix ;)

tests/testthat/test-borders.R

tests/testthat/test-captions-rmd.R

tests/testthat/test-md-captions.R

davidgohel · 2024-03-14T08:19:35Z

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 · 2024-03-14T10:02:48Z

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

Melkiades · 2024-03-14T11:26:23Z

tests/testthat/setup.R

+  # Construct the path to the _snaps folder
+  path_to_snaps <- file.path(path_to_testthat, "_snaps")
+
+  file.copy(folder_to_copy, path_to_snaps, recursive = TRUE, overwrite = TRUE)


I need to find a way to remove them at the end of the testing

No, drop them. Otherwise I fear we end with a "workaround" and not a clean solution - I will add them later

I may add skip_on_ci on the general call so we do not lose the code maybe?

Or do you mean to remove the files only? I think it is complicated anyway to have a feedback from test to inst if things are changed, it makes it a bit too manual does it?

It's on github, nothing is lost. I will isolate the files on my computer and maybe put them in inst later.

After all, it was only to me a way to check all is ok visually before git-comiting. They are not managed by CI/CD, they sometimes fail (I think because the license check takes too much time with my Office 365 account), etc.

The files that could be deleted are:

"test-borders.R"

"test-md-captions.R"

"test-as_flextable.R"

and then the directory tests/testthat/_snaps

davidgohel · 2024-03-14T11:27:10Z

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 :)

tests/testthat/setup.R

codecov-commenter · 2024-03-14T12:23:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

davidgohel · 2024-03-15T13:57:22Z

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 · 2024-03-15T15:00:16Z

tests/testthat/setup.R

+    # Folder where the snapshots are stored
+    folder_to_copy <- system.file("snapshots_for_manual_tests", package = "flextable")
+
+    # Get the path to the tests/testthat directory
+    path_to_testthat <- system.file("tests", "testthat", package = "flextable")
+
+    # Construct the path to the _snaps folder
+    path_to_snaps <- file.path(path_to_testthat, "_snaps")
+
+    file.copy(folder_to_copy, path_to_snaps, recursive = TRUE, overwrite = TRUE)


I still need to fix this, but it is dependent on the flag do_manual_msoffice_snapshot_testing that can be set TRUE on local, so it should not be a problem on ci

Melkiades · 2024-03-15T15:06:09Z

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 · 2024-03-20T10:13:59Z

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 · 2024-03-20T11:03:47Z

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 · 2024-03-20T12:07:08Z

tests/testthat/setup.R

+do_manual_msoffice_snapshot_testing <- FALSE
+copy_back_new_snapshots <- FALSE # if snapshots are updated can be rewritten back


These are the main drivers of the manual snapshot testing

Melkiades · 2024-03-20T12:07:49Z

tests/testthat/setup.R

+  invisible(TRUE)
+}
+
+handle_manual_snapshots <- function(snapshot_folder, snapshot_name) {


This function copy snapshots into the folder and removes them with withr::defer

Melkiades · 2024-03-20T12:08:29Z

tests/testthat/setup.R

+  withr::defer(
+    {
+      snap_file <- file.path(path_to_snaps, snapshot_name)
+      if (copy_back_new_snapshots) {


The copying back is available here, it is the only reason to do defer here

Melkiades · 2024-03-20T12:11:55Z

@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 · 2024-03-21T21:16:07Z

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

I'm merging so we can move forward

Melkiades added 2 commits March 13, 2024 11:17

cleaning library calls

60ef9c2

small fix for local

bcfa423