Skip to content

gforce coverage add newlines again#5203

Closed
ben-schwen wants to merge 2 commits intomasterfrom
gforce_coverage_newlines
Closed

gforce coverage add newlines again#5203
ben-schwen wants to merge 2 commits intomasterfrom
gforce_coverage_newlines

Conversation

@ben-schwen
Copy link
Copy Markdown
Member

@ben-schwen ben-schwen commented Oct 8, 2021

Follow up to #5192 and related to #4301 (comment)

AFAIU the nrow != length error occurs if the passed grpsize to gforce differs from the length of the object passed to the gfunction itself. Since the gfunctions themselves are only called internally after internal gforce optimization happening, this should never happen internally. However, users could still try to call data.table:::gsum on their own which might even work for some cases, e.g.

DT = data.table(a=1:5, b=1:5, c=1)
DT[, lapply(.SD, data.table:::gmean), c, verbose=TRUE]

I'm not sure if exposing the gforce functions at the beginning of tests.Rraw is a good idea but only other option I can think of is to parse them later with data.table::: which contradicts

The string "data.table::" (which covers "data.table:::" too) should exist nowhere else in this file

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 8, 2021

Codecov Report

Merging #5203 (368ce4b) into master (96cdef6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5203   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files          77       77           
  Lines       14529    14536    +7     
=======================================
+ Hits        14458    14465    +7     
  Misses         71       71           
Impacted Files Coverage Δ
src/gsumm.c 100.00% <100.00%> (ø)

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 96cdef6...368ce4b. Read the comment docs.

@mattdowle
Copy link
Copy Markdown
Member

mattdowle commented Oct 15, 2021

However, users could still try to call data.table:::gsum on their own which might even work for some cases

They could, but they shouldn't. If it doesn't crash and even then produces the correct result, then it's 1 in 100 luck. The gforce() prep function must be called first (as is done by [.data.table), then the g* functions reuse that state.

I know a few users look at and search tests.Rraw to glean tested usage. We don't want to give even the slightest indication that calling the internal g* functions directly might be ok.

On these errors you're trying to cover :

if (nrow != n) error(_("nrow [%d] != length(x) [%d] in %s"), nrow, n, ...);

that nrow is a static global assigned by that prep function gforce(). So it's a basic sanity check that gforce was called first correctly. If you want to, these error messages could be moved to the new line, "Internal error: ..." added to the beginning of the error message, and marked # nocov. But I'd really prefer this time be used to investigate within-line C coverage as per my comment you linked to up top. If folk think that within-line-C-coverage is unrealistic, then I just need something to point to and then I'll be happier to spend time on changes like this.

@mattdowle mattdowle closed this Oct 15, 2021
@ben-schwen ben-schwen deleted the gforce_coverage_newlines branch October 21, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants