insightsengineering / teal.data

Data model for teal applications
https://insightsengineering.github.io/teal.data/
Other
8 stars 7 forks source link

Fix removal of curly brackets in `get_code` for code longer than 1 #312

Closed m7pr closed 4 months ago

m7pr commented 4 months ago

Solves issues like this fail in R CMD CHECK for teal https://github.com/insightsengineering/teal/actions/runs/9064624851/job/24903280564?pr=1217#step:41:152

m7pr commented 4 months ago

Hey @kartikeyakirar looks like during https://github.com/insightsengineering/teal.data/pull/311 we didn't cover this case and some R CMD Checks are failing. https://github.com/insightsengineering/teal/actions/runs/9064624851/job/24903280564?pr=1217#step:41:152

Would you mind reviewing?

github-actions[bot] commented 4 months ago

Unit Tests Summary

  1 files   14 suites   2s :stopwatch: 196 tests 194 :white_check_mark: 2 :zzz: 0 :x: 269 runs  267 :white_check_mark: 2 :zzz: 0 :x:

Results for commit 9c8f3a6c.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 4 months ago

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  --------------------
R/cdisc_data.R                       1       0  100.00%
R/deprecated.R                      57      57  0.00%    19-344
R/dummy_function.R                   2       2  0.00%    14-15
R/formatters_var_labels.R           61       0  100.00%
R/join_key.R                        38       0  100.00%
R/join_keys-c.R                     12       0  100.00%
R/join_keys-extract.R              128       0  100.00%
R/join_keys-names.R                 15       0  100.00%
R/join_keys-parents.R               30       0  100.00%
R/join_keys-print.R                 45       0  100.00%
R/join_keys-utils.R                 73       3  95.89%   35-38
R/join_keys.R                       21       0  100.00%
R/teal_data-class.R                 25       1  96.00%   69
R/teal_data-datanames.R             10       0  100.00%
R/teal_data-get_code.R              14       0  100.00%
R/teal_data-show.R                   4       4  0.00%    14-19
R/teal_data.R                       30      16  46.67%   33, 36-42, 52-58, 61
R/testhat-helpers.R                 26       0  100.00%
R/topological_sort.R                32       0  100.00%
R/utils-get_code_dependency.R      187       1  99.47%   282
R/verify.R                          42      11  73.81%   65, 95-99, 102-106
TOTAL                              853      95  88.86%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 9c8f3a6c500f97fc39c6182cfeaecdfb4b69a96c

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

vedhav commented 4 months ago

Can we add a simple unit test for the future?

Perhaps, something like this?

testthat::test_that("get_code_dependency returns the dependant code for the dataset name", {
  code <- c("IRIS <- head(iris)", "MTCARS <- head(mtcars); FULL_IRIS <- iris")
  expect_identical(
    get_code_dependency(code, "IRIS"),
    "IRIS <- head(iris)"
  )
  expect_identical(
    get_code_dependency(code, "FULL_IRIS"),
    "FULL_IRIS <- iris"
  )
})

testthat::test_that("get_code_dependency removes {} if the code is bounded by them", {
  code <- c("IRIS <- head(iris)", "{MTCARS <- head(mtcars)}")
  expect_identical(
    get_code_dependency(code, "MTCARS"),
    "MTCARS <- head(mtcars)"
  )
})
m7pr commented 4 months ago

@vedhav I added a test for get_code. The function that you mention get_code_dependency is an internal. We do not test internals.

github-actions[bot] commented 4 months ago

Unit Test Performance Difference

Additional test case details | Test Suite | $Status$ | Time on `main` | $±Time$ | Test Case | |:-----|:----:|:----:|:----:|:-----| | get_code | 👶 | | $+0.01$ | handles_the_code_of_length_1_when_at_least_one_is_enclosed_in_curly_brackets |

Results for commit 0facd4274b722b3c3157a38a9332c66c72e84830

♻️ This comment has been updated with latest results.

m7pr commented 4 months ago

Sorry this got merged during auto-merge strategy on when I accepted the suggestion lol I continued with one more commit in here https://github.com/insightsengineering/teal.data/pull/314/files