Recursive test_r() #433

radbasa · 2023-01-27T06:07:05Z

Changes

A unit test handler than can accept a vector of paths (a combination of files and directories) to run tests on.

test_r() defaults to running testthat::test_file() on all *.R files found inside tests/testthat/ directory and nested directories. test_file() is intelligent enough to not do anything (returns NULL) if it does not find any tests.

Ways of using test_r():

test_r()
test_r(paths = "tests/testthat/test-main.R")
test_r(paths = "tests/testthat/logic")
test_r(paths = c("tests/testthat/test-main.R", "tests/testthat/logic")

Removed R6. Now just a collection of functions.

In addition to paths, test_r() accepts:

inline_issues - defaults to FALSE, switches where failures and skipped test messages are located. TRUE mimics testthat::test_dir() behavior.
raw_testthat_output - defaults to FALSE which causes test_r() to return a single data.frame. TRUE causes test_r() to return a list of testthat::test_file() results which are lists containing data of test results.

Also added:

duration report with a default min_time = 0.1

Not (yet) added:

testthat encouragement message

~~Added a unit test handler that can run tests on:~~

~~single test file~~
~~single test dir (non-recursive)~~
~~recursive test dirs from a base path~~

A lot of code taken from testthat internal functions to have the same look and feel. May need attribution.

~~I am open to suggestions on how to write unit tests for this.~~ Unit tests written. May need more detailed tests (snapshots).

Addresses Issue #39

How to test

unit tests for test_r() are available in tests/testthat/test-test_r.R. It uses tests/testthat/test_recursive as its sample test files and directories
~~rhino::test_r() - recursive run on all valid test-*.R files in tests/testthat, if there are no nested test folders, just runs the basic testthat::test_dir("tests/testthat")~~
~~rhino::test_r(recursive = FALSE) - equivalent to testthat::test_dir("tests/testthat")~~
~~rhino::test_file("tests/testthat/main.R") - equivalent to testthat::test_file("tests/testthat/main.R")`~~

Covr

~~Appsilon/covr will need to be updated to handle nested test folders~~ Done. Appsilon/rhino@feature/covr-support version 1.3.0.9003

codecov-commenter · 2023-01-27T06:10:18Z

Codecov Report

Merging #433 (29a5d59) into main (6696e81) will increase coverage by 26.94%.
The diff coverage is 92.95%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##             main     #433       +/-   ##
===========================================
+ Coverage   11.90%   38.85%   +26.94%     
===========================================
  Files           8        9        +1     
  Lines         294      435      +141     
===========================================
+ Hits           35      169      +134     
- Misses        259      266        +7

Impacted Files	Coverage Δ
R/test_helpers.R	`92.36% <92.36%> (ø)`
R/tools.R	`11.82% <100.00%> (+11.82%)`	⬆️
R/app.R	`32.65% <0.00%> (+4.08%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

radbasa · 2023-01-27T06:19:17Z

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: rhino
 Title: A Framework for Enterprise Shiny Applications
-Version: 1.3.0
+Version: 1.3.0.8001


Avoiding version conflict with the covr support branch.

radbasa · 2023-01-27T06:23:36Z

R/recursive_unit_tests.R

+        valid_paths <- unique(
+          fs::path_dir(
+            fs::dir_ls(path = private$path,
+                       regexp = private$filter,
+                       recurse = private$recursive, type = "file")
+          )
+        )
+
+        private$valid_paths <- valid_paths[order(valid_paths)]


Because testthat::test_dir() throws an error if given a path without a valid test file, we get a list of valid test files, then get their paths.

radbasa · 2023-01-27T06:25:05Z

R/recursive_unit_tests.R

+r_cmd_check_fix <- function() {
+  testthat::test_check()
+}


Without this, I was getting an R CMD CHECK note Namespace in Imports field not imported from: 'testthat'

kamilzyla

Hey Rodrigo, many thanks for this PR! I really appreciate that you dived into this issue 😃 👍 I'm doing a detailed code review yet, as I'd like to discuss a bunch of high-level points first.

Output

The most important point for me is the console output of test_r(). With this implementation we'll print test results for each directory and a summary at the end. This leads to a ton of horizontal lines and partial summaries; I find it very difficult to read.

Ideally the output would look pretty much like it did before: just a single table with all tests and one summary line at the end. Is it feasible?

`test_r()` API

Ideal API IMO:

Accept a vector in paths (files and directories with tests to run).
Drop recursive (when would we need something else than TRUE?)
Replace ... with filter (what other use cases would it have?).

Additionally:

Apply filter after stripping test- and .R.
Don't require test- prefix for files (is it possible?).

Implementation / approach

Using getFromNamespace() is risky, just like :::. CRAN reviewers might not accept it; it will leave us prone to internal changes in testthat.
I think, ideally, testthat would export a function which can run all tests in the given file and return results in a format which can be further processed (e.g. data frame). How about submitting a patch for testthat?
I can see how R6 helps us to avoid introducing too many functions to the outside world 👍 However, storing the arguments as private class fields makes it somewhat difficult to reason about the object state. Passing stuff around via arguments and return values would probably make for a more readable code. Perhaps we don't need R6 then?

Code style

See my comments in the code.

Final words

Again, thanks a lot for working on this! ❤️ Some issues might be easier to discuss in person; LMK if you'd like to have a call. Also LMK if you need some support getting more time reserved to work on this 🙂

kamilzyla · 2023-01-31T15:04:05Z

R/recursive_unit_tests.R

+      test_results <- private$run_recursive_test_dir(...)
+
+      private$show_summary(test_results)
+
+      private$show_final_line(test_results)


Recommendation: Use blank lines sparingly, to separate logical blocks of code.

kamilzyla · 2023-01-31T15:10:15Z

R/recursive_unit_tests.R

+        summary_line(final_line_results[["failed"]],
+                             final_line_results[["warning"]],
+                             final_line_results[["skipped"]],
+                             final_line_results[["passed"]])


Recommendation: Use the following indentation style when arguments don't fit on one line:

func( long_arg_1, # All arguments indented with 2 spaces. long_arg_2 ) # Closing parenthesis on new line, with the same indentation as `func`.

This way if func is renamed there is no need to adjust the indentation of its arguments, which is easy to forget about and leads to larger / not meaningful diffs (harder to review).

radbasa · 2023-02-01T11:13:54Z

Hi Kamil,

Thank you for the code review.

Output

The output looks like that now because all we are doing now is running test_dir() multiple times. And we get the normal stdout output of test_dir(). What we can do is to use the SilentReporter of testthat and produce our own test result table. We can also have all the failures in one group. We will need to copy-paste more display code from testthat.

API

paths as a vector of files and directories is doable.
recursive = TRUE: We might want to run tests on only one directory, and not the nested directories.
...: This is passed to test_dir() which can accept many other arguments. It also passes ... to testthat::find_test_scripts() which has its own set of arguments and also passes ... to testthat:::filter_test_scripts() and also finally passes ... to grepl().
test-.+\\.R$ filter: testthat::test_dir() when given a directory that does not contain at least one test-*.R file throws an error. The test_dir() filter argument matches file names after stripping test- and .R. It will only accept test-*.R files, unlike testthat::test_file() which will work with any file name. This is why when we go through the directories recursively we actually look for test-*.R files and get the unique directories.

Error in `testthat::test_dir()`:
! No test files found

getFromNamespace(): in a previous commit, I just copied over the code from testthat. I can revert to that.
fork and patch r-lib/testthat: @jakubnowicki 's requirement was to avoid modifying testthat and do everything in rhino.
But, we can still make rhino::test_r() return a data.frame, or a list of data.frames (the raw return output of testthat::test_dir() and test_file().
We can strip out the functions from the R6 class.

radbasa · 2023-02-02T12:15:57Z

Massive update

…eload() calls in test files

…est-directories

radbasa · 2023-02-14T08:42:13Z

Retracting this PR in favor of forking testthat itself Appsilon/testthat#2

radbasa added 2 commits January 27, 2023 13:58

adding test handler for single file, single dir, or recursive dir.

0731e09

prevent multiple paths

91032d2

radbasa requested review from jakubnowicki and kamilzyla January 27, 2023 06:07

radbasa self-assigned this Jan 27, 2023

a dummy function to pass r cmd check

514e457

radbasa commented Jan 27, 2023

View reviewed changes

radbasa added the type: feature Major new functionality label Jan 27, 2023

radbasa added 2 commits January 31, 2023 13:38

return error context as rhino::test_r()

4d693d2

some code cleanup

9eb3a09

kamilzyla requested changes Jan 31, 2023

View reviewed changes

radbasa added 6 commits February 2, 2023 15:47

run tests by file. duplicate testthat progress reporter look and feel

10f600f

added skip test handling. modified docs.

1c4c911

renamed test helper file

54a0703

adjust whitespace

ae9ccde

cleanup styling

6e57137

documentation cleanup

5cd4927

radbasa added 7 commits February 2, 2023 20:43

added warning messages

7224d28

updated documentation for warnings

bc31fc5

clean environment of box modules before running tests to avoid box::r…

45e7b26

…eload() calls in test files

Merge branch 'fix/purge-box-cache-in-test_r' into feature/recursive-t…

a15c2b3

…est-directories

rationalize version numbers and news entry

01b62bf

Merge branch 'main' into feature/recursive-test-directories

29a5d59

fix some ci fails (lint and spelling)

babfb81

radbasa mentioned this pull request Feb 6, 2023

Give test_dir() a recursive option Appsilon/testthat#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursive test_r() #433

Recursive test_r() #433

radbasa commented Jan 27, 2023 •

edited

codecov-commenter commented Jan 27, 2023 •

edited

radbasa Jan 27, 2023

radbasa Jan 27, 2023 •

edited

radbasa Jan 27, 2023

kamilzyla left a comment

kamilzyla Jan 31, 2023

kamilzyla Jan 31, 2023

radbasa commented Feb 1, 2023 •

edited

radbasa commented Feb 2, 2023

radbasa commented Feb 14, 2023

Recursive test_r() #433

Are you sure you want to change the base?

Recursive test_r() #433

Conversation

radbasa commented Jan 27, 2023 • edited

Changes

How to test

Covr

codecov-commenter commented Jan 27, 2023 • edited

Codecov Report

radbasa Jan 27, 2023

Choose a reason for hiding this comment

radbasa Jan 27, 2023 • edited

Choose a reason for hiding this comment

radbasa Jan 27, 2023

Choose a reason for hiding this comment

kamilzyla left a comment

Choose a reason for hiding this comment

Output

test_r() API

Implementation / approach

Code style

Final words

kamilzyla Jan 31, 2023

Choose a reason for hiding this comment

kamilzyla Jan 31, 2023

Choose a reason for hiding this comment

radbasa commented Feb 1, 2023 • edited

Output

API

radbasa commented Feb 2, 2023

radbasa commented Feb 14, 2023

radbasa commented Jan 27, 2023 •

edited

codecov-commenter commented Jan 27, 2023 •

edited

radbasa Jan 27, 2023 •

edited

`test_r()` API

radbasa commented Feb 1, 2023 •

edited