Bunch of coverage tests by MichaelChirico · Pull Request #3168 · Rdatatable/data.table

MichaelChirico · 2018-11-30T12:30:49Z

Closes #3167
Closes #3169

Ran out of steam to do more tests for now.

I'm not sure if we have any way of doing the !cedta() tests so I just nocov'd them.

codecov · 2018-11-30T12:55:32Z

Codecov Report

Merging #3168 into master will increase coverage by 0.96%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #3168      +/-   ##
==========================================
+ Coverage   92.34%   93.31%   +0.96%     
==========================================
  Files          61       61              
  Lines       11592    11588       -4     
==========================================
+ Hits        10705    10813     +108     
+ Misses        887      775     -112

Impacted Files	Coverage Δ
R/duplicated.R	`94.59% <ø> (+16.67%)`	⬆️
R/setkey.R	`98.69% <100%> (+15.36%)`	⬆️
R/frank.R	`100% <100%> (+17.56%)`	⬆️
R/IDateTime.R	`95.91% <66.66%> (+10.3%)`	⬆️
R/groupingsets.R	`95.5% <87.5%> (+11.23%)`	⬆️
R/last.R	`61.11% <0%> (+5.55%)`	⬆️
R/fcast.R	`89.94% <0%> (+6.14%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da98fb2...b02b929. Read the comment docs.

jangorecki

LGTM

MichaelChirico · 2018-11-30T18:37:14Z

R/setkey.R

  if (is.na(decreasing) || !is.logical(decreasing)) stop("'decreasing' must be logical TRUE or FALSE")
  cols = substitute(list(...))[-1L]
-  if (identical(as.character(cols),"NULL") || !length(cols)) return(NULL) # to provide the same output as base::order
+  if (identical(as.character(cols),"NULL") || !length(cols) || (length(cols) == 1L && !nzchar(cols))) return(NULL) # to provide the same output as base::order


this covers forder(DT, ) case, which errored otherwise later on and with a weird message

MichaelChirico · 2018-11-30T18:37:52Z

R/setkey.R


 setorderv <- function(x, cols, order=1L, na.last=FALSE)
 {
+  if (missing(cols)) cols = colnames(x)


As discussed in #3169; not setting as default argument for now as that may be a breaking change.

what breaking change it would be precisely?

I dunno. I guess adding a default argument is different from removing a default argument. Will fix

…o dup_coverage

MichaelChirico · 2018-12-01T03:16:58Z

@jangorecki 1960.92 gave this output locally:

data.table(grouping = c(NA_integer_, NA_integer_), V1 = c(5, 5))

but all the CI seems to give the output

data.table(grouping = c(NA_integer_, 0L), V1 = c(5, 5))

Can you confirm which is right/why the difference?

Full code:

DT = data.table(
  color = c("yellow", "red", "green", "red", "green", "red",
            "yellow", "yellow", "green", "green", "green", "yellow",
            "red", "yellow", "red", "green", "yellow", "red", "yellow",
            "red", "green", "yellow", "green", "green"),
  year = structure(c(15340, 15340, 14975, 15706, 15706, 15340,
                     16436, 15340, 15340, 14975, 16436, 15706,
                     16436, 15340, 14975, 14975, 16071, 15340,
                     15706, 16071, 15706, 15340, 16436, 16071), class = "Date"),
  status = structure(c(4L, 3L, 4L, 3L, 2L, 1L, 3L, 4L, 4L, 3L, 4L, 4L,
                       4L, 4L, 1L, 3L, 3L, 2L, 1L, 2L, 3L, 4L, 2L, 4L),
                     .Label = c("active", "archived", "inactive", "removed"),
                     class = "factor"),
  amount = c(1L, 4L, 2L, 3L, 1L, 5L, 1L, 1L, 4L, 2L, 3L, 1L,
             5L, 4L, 2L, 2L, 4L, 3L, 3L, 2L, 4L, 4L, 1L, 2L),
  value = c(2.5, 2, 3, 3, 2.5, 3.5, 2.5, 3.5, 3, 2.5, 3.5, 2.5, 2,
            2.5, 3, 3, 3, 3, 3, 3, 2, 2.5, 3, 3)
)
groupingsets(DT[ , .(amount, value)], j = 5, by = character(0L),
             sets = list(character()), id=TRUE)

jangorecki · 2018-12-01T03:45:03Z

I get locally

   grouping V1
1:       NA  5
2:        0  5

can you try R --vanilla?
this is some crazy edge case unit test :O
NA in grouping (aggregation level identifier) column is suspicious

MichaelChirico · 2018-12-01T04:05:46Z

Doesn't close #3172... I don't know what's going on here, some where primitive-base-S4 thing going on I can't seem to fix:

x = as.IDate('2018-08-01')
# should be triggering the inherits(e2, 'difftime') branch of -.IDate but..
x - difftime(x, x - 1)

Warning message:
Incompatible methods ("-.IDate", "Ops.difftime") for "-"

Might need to do some setMethod approach?? I don't know how R is parsing this

MichaelChirico · 2018-12-01T04:09:56Z

@jangorecki yea it took me a while to cook this one up... R --vanilla I get same output.

Rstudio sessionInfo():

Details

R version 3.5.1 (2018-07-02)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS High Sierra 10.13.6

Matrix products: default
BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] data.table_1.11.9

loaded via a namespace (and not attached):
[1] compiler_3.5.1 tools_3.5.1    yaml_2.2.0

R --vanilla sessionInfo():

Details

R version 3.5.1 (2018-07-02)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS High Sierra 10.13.6

Matrix products: default
BLAS: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRblas.0.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] data.table_1.11.9

loaded via a namespace (and not attached):
[1] compiler_3.5.1 tools_3.5.1

MichaelChirico · 2018-12-01T04:22:08Z

@jangorecki shall I just delete that test here? since presumably it's testing wrong behavior, just leave that test for #3173

jangorecki · 2018-12-01T04:28:55Z

Yes, please remove that test from here. I already included it in another branch. Any idea what was the profile config that was messing up output from NA, 0 to NA, NA?

MichaelChirico · 2018-12-01T07:48:58Z

Any idea what was the profile config that was messing up output from NA, 0 to NA, NA?

not a clue...

mattdowle · 2018-12-05T01:38:09Z

Looks great. Nearly +1% coverage!
I moved the ::: to the right place.
3 test are failing: 1962.86, 1962.92 and 1962.108. I had a quick look but didn't immediately see what was up.

MichaelChirico · 2018-12-05T02:59:36Z

Fixed the failing tests. One was giving me whack-a-mole problems so I just dropped it. If there's still some coverage missed we'll get back to it eventually.

MichaelChirico · 2018-12-05T06:14:06Z

@mattdowle not sure how patch could possibly be showing what it is on the diff target given the number of tests added... guess i still don't understand the inner workings of codecov well enough

mattdowle · 2018-12-05T19:25:19Z

@MichaelChirico I'm not really sure either but maybe new added lines (like all the new tests you added here) aren't counted as "diff" (changed lines) for the purpose of that 89% patch coverage figure. If you click the "Diff" tab on codecov.io, it is showing 5 stop()s and 1 return() that aren't covered. Those do seem to be changed lines in this PR. If you can cover those 6 lines please then patch coverage should go to 100% I think.

MichaelChirico · 2018-12-06T02:54:58Z

@mattdowle I see... a couple of those were branches that I tried to take a swing at and couldn't hit for the life of me...

So I gave up on adding tests there for now (resisted the urge to nocov) them in fact...

mattdowle · 2018-12-06T06:48:51Z

@MichaelChirico Ok I see. Agree - good approach i.e. those 6 will remain in the project uncovered list until someone can get to them, then. Will merge.

jangorecki · 2020-04-11T10:06:35Z

inst/tests/tests.Rraw

+## uniqlist.R
+test(1962.10, uniqlist(1:5),
+     error = 'not type list')
+test(1962.11, uniqlist(list()), list(0L))


@MichaelChirico This is an example where blind aiming for higher code cov can cause an issue. AFAIU uniqlist was used more widely before, as of now it is being used in very few places. None of those, AFAICT, requires this case to be handled in the way as it is defined by 1962.11 test. So now in #4372 I am changing this behaviour and because of this test (that has been added in a batch, for code cov, and not really because it was required), my PR might look like a breaking change.
I am just elaborating, so that we can be aware it might be better not to increase codecov like this, because we adapt use cases (tests) to the code, and it should be the opposite, code should be adapted to a uses cases (tests).

i think it's fine anyway. the change is in fact breaking right? just that we don't care if internal behavior changes. easy enough to note on the new PR that the altered tests are of internal behavior.

Michael Chirico added 2 commits November 30, 2018 19:14

complete coverage of duplicated.R

47a3561

Bunch of coverage tests; also closes #3167

d96d759

jangorecki approved these changes Nov 30, 2018

View reviewed changes

More tests, including closing #3169

4c82dfb

MichaelChirico commented Nov 30, 2018

View reviewed changes

Michael Chirico added 2 commits December 1, 2018 10:41

Merge branch 'master' of https://github.com/Rdatatable/data.table int…

6b8ec50

…o dup_coverage

fix test?

c4774ce

MichaelChirico force-pushed the dup_coverage branch from 3a9e303 to c4774ce Compare December 1, 2018 02:54

reorder tests

bb98630

MichaelChirico changed the title ~~Bunch of coverage tests; also closes #3167~~ Bunch of coverage tests; also closes #3167 and #3169 Dec 1, 2018

MichaelChirico mentioned this pull request Dec 1, 2018

-.IDate fails when e2 numeric #3172

Closed

IDateTime tests and some progress on #3172

7f1a27a

jangorecki mentioned this pull request Dec 1, 2018

grouping sets incorrectly handle constant value in j #3173

Closed

Michael Chirico added 2 commits December 1, 2018 15:41

Drop test of bug given #3173, #3174

491b201

Complete #3169 - set cols default in setorderv, clarify in manual

26248a2

Merge branch 'master' into dup_coverage

45a44e4

mattdowle changed the title ~~Bunch of coverage tests; also closes #3167 and #3169~~ Bunch of coverage tests Dec 5, 2018

mattdowle added this to the 1.12.0 milestone Dec 5, 2018

Moved ::: up to top section

dbc3a09

fix tests

f7d89e9

mattdowle and others added 2 commits December 5, 2018 15:49

Merge branch 'master' into dup_coverage

e5a3ee4

merging from master

55d8838

Michael Chirico and others added 2 commits December 6, 2018 10:55

merging again

9bde8ac

Merge branch 'master' into dup_coverage

b02b929

mattdowle merged commit eec2e6e into master Dec 6, 2018

mattdowle deleted the dup_coverage branch December 6, 2018 07:05

mattdowle added a commit that referenced this pull request Dec 6, 2018

line length in .Rd, and tolerance needed in two tests from #3168

f14cb11

mattdowle mentioned this pull request Dec 6, 2018

fixed IDateTime.R use of base::.Date #3191

Merged

MichaelChirico mentioned this pull request Jan 11, 2019

test 1960 still failing (grouping sets output mismatch) #3267

Closed

jangorecki reviewed Apr 11, 2020

View reviewed changes

Conversation

MichaelChirico commented Nov 30, 2018 • edited by mattdowle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jangorecki left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

jangorecki Dec 1, 2018

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Dec 1, 2018

Choose a reason for hiding this comment

Uh oh!

MichaelChirico commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jangorecki commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelChirico commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelChirico commented Dec 1, 2018

Uh oh!

MichaelChirico commented Dec 1, 2018

Uh oh!

jangorecki commented Dec 1, 2018

Uh oh!

MichaelChirico commented Dec 1, 2018

Uh oh!

mattdowle commented Dec 5, 2018

Uh oh!

MichaelChirico commented Dec 5, 2018

Uh oh!

MichaelChirico commented Dec 5, 2018

Uh oh!

mattdowle commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelChirico commented Dec 6, 2018

Uh oh!

mattdowle commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jangorecki Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MichaelChirico commented Nov 30, 2018 •

edited by mattdowle

Loading

codecov bot commented Nov 30, 2018 •

edited

Loading

MichaelChirico commented Dec 1, 2018 •

edited

Loading

jangorecki commented Dec 1, 2018 •

edited

Loading

MichaelChirico commented Dec 1, 2018 •

edited

Loading

mattdowle commented Dec 5, 2018 •

edited

Loading

mattdowle commented Dec 6, 2018 •

edited

Loading