Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotfix: Fix regression in how test depth is calculated; add exec index #485

Merged
merged 6 commits into from
Jul 23, 2021
Merged

Hotfix: Fix regression in how test depth is calculated; add exec index #485

merged 6 commits into from
Jul 23, 2021

Conversation

dgkf
Copy link
Contributor

@dgkf dgkf commented Jul 22, 2021

This is a little bit of a dual part PR - first a fix to a regression introduced over the course of preparing #463, and the other piece is a small feature enhancement that enables a lot of powerful functionality.

Depth calculation fix

Between the initial PR for #463 to the final commit, I went from just recording the final test frame to instead recording the entire test call stack up until the first covr::count call. Unfortunately, I forgot to update the way that the stack depth of the test trace was calculated. This has been fixed now by taking the length of the stack trace, instead of (what was) the index of the frame to begin on.

-  depth_into_pkg <- length(sys.calls()) - .current_test$frame - n_calls_into_covr + 1L
+  depth_into_pkg <- length(sys.calls()) - length(.current_test$frames) - n_calls_into_covr + 1L

Unfortunately, this was silently causing incorrect depth because of rbind's value recycling. (effectively doing the same as rbind(c(1, 2), c(3, NULL))) (location in diff).

All is good now, and I added in some tests to avoid this from happening in the future.

Feature Addition: Test execution index

As well, I added in an index for each trace. I'm pretty excited to have this introduced. Although it's just a tiny bit of added information, it effectively means that we can reconstruct the execution path that a test took while evaluating which opens up a lot of opportunities for analyzing the test traces.

In practice we get something like this (below), where i records the order in which each trace is executed by each test. Combining this matrix across traces and sorting by "test" and "i" would let us see the order of coverage counts executed by a test.

#' # $`source.Ref2326138c55:3:8:3:8:8:8:3:3`
#' #      test depth i
#' # [1,]    1     1 1
#' # [2,]    1     2 3

Materially, this means storing an extra column to the tests matrix for each trace. It comes at no substantial computational or object size cost.


  • Updated ?covr.record_tests documentation to describe new data column
  • Added tests
  • Bumped DESCRIPTION dev version
  • Added reference to PR in NEWS entry regarding these changes

@jimhester
Copy link
Member

LGTM, is this ready to merge then?

@dgkf
Copy link
Contributor Author

dgkf commented Jul 23, 2021

Yes - it's good to go. Thanks @jimhester

@jimhester jimhester merged commit 9133a91 into r-lib:master Jul 23, 2021
@jimhester
Copy link
Member

Hey! Thanks!

@dgkf dgkf deleted the hotfix/test-trace-depth branch July 23, 2021 16:11
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.

None yet

2 participants