Skip to content

Traceability matrix adjustment #71

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

Merged
merged 14 commits into from
Sep 10, 2024
Merged

Traceability matrix adjustment #71

merged 14 commits into from
Sep 10, 2024

Conversation

barrettk
Copy link
Collaborator

Traceability matrix adjustment:

  • we noticed that mrgsolve was not correctly mapping the documentation and code files to the exports in cases were setMethod() and other forms of definition were used.

  • This patch includes two core changes, a bug fix, and additional items that needed to be changed because of other two.

    • exports referenced in Rd files are now split on commas, allowing them to be properly left joined back to the exports data
    • documentation and code files are now collapsed in a list, as mrgsolve has numerous cases of multiple scripts or documentation per export
  • The method for checking for missing links has been fixed and moved to its own function for easier adjustment. I noticed that this wasnt actually being called in most cases, as empty missing links were empty strings rather than NA characters in most cases. I need to get feedback on how we want to display these still, but I think transitioning missing links to empty strings should happen in the formatting functions anyways.

@barrettk
Copy link
Collaborator Author

barrettk commented Aug 26, 2024

Some notes:

  • I figured out at least one reason test_files were not getting indented in the trace matrix. flextable has its own wrapping built in once strings reach a specified column width. In this case it was about ~39 characters pre-indent and now 35 characters with the indent. By changing the specified width in wrap_text(), new lines should now be properly indented when new lines are introduced due to longer strings. Given the reason it wasnt working before, Im opting to not write a test, as I think this would be difficult for minimal reward.
  • Most Importantly: local devtools::test() is failing, yet local R CMD Check and CI are passing. I havent been able to discern where the difference is coming from, but I can tell that the setup.R script is failing to properly score or setup packages ahead of time, causing a lot of test-result tests to fail, yielding downstream implications.
    • I'll keep looking a bit more, but will leave it up to the reviewer whether its important we address this ahead of merging. If they could confirm if they are seeing similar results that would also be helpful.

cc @seth127 @kyleam

@barrettk barrettk requested a review from seth127 August 27, 2024 14:49
 - we noticed that mrgsolve was not correctly mapping the documentation and code files to the exports in cases were setMethod() and other forms of definition were used.

 - This patch includes two core changes, a bug fix, and additional items that needed to be changed _because_ of other two.
   - exports referenced in Rd files are now split on commas, allowing them to be properly left joined back to the exports data
   - documentation and code files are now collapsed in a list, as mrgsolve has numerous cases of multiple scripts or documentation per export

 - The method for checking for missing links has been fixed and moved to its own function for easier adjustment. I noticed that this wasnt actually being called in most cases, as empty missing links were empty strings rather than NA characters in most cases. I need to get feedback on how we want to display these still, but I think transitioning missing links to empty strings should happen in the formatting functions anyways.

 - most tests have been properly adjusted and work appropriately. However, the last test in test-make-traceability-matrix now fails. Interestingly I dont think it was introduced in this PR, so need to determine what to do here.
 - see inline comments for more details about specific widths that were chosen.
For exports that span more than a page (usually due to many tests), split the entry into `export` and `export (cont.)`. This is necessary to ensure tables do not overflow into the footer
@seth127
Copy link
Collaborator

seth127 commented Aug 30, 2024

@barrettk I looked at this a bit yesterday. The rendered docs for a few packages that I spot-checked (mrgsolve, bbr, bbi and pkgr with the new "external scores" functionality) all looked great. That said...

Most Importantly: local devtools::test() is failing, yet local R CMD Check and CI are passing

Yes, I think that is a problem. I saw the same thing (on Metworx 24.04) yesterday. Although I actually had one failure and one warning on my local devtools::check() too (and a bunch on test()).

Let me know when you get a chance to look into that deeper. And make sure to pull the most recent dependencies. As you said, I don't immediately see how these failures are related to any of the changes here, so my first guess is that one of our deps updated and broke something. Maybe try this branch with the deps coming from an older MPN snapshot (instead of CRAN) and see if that makes a difference? Thanks.

@barrettk
Copy link
Collaborator Author

barrettk commented Sep 10, 2024

Discussed offline, but posting here for posterity:

I determined the test issues were related to the Metworx Blueprint. @seth127 and I discussed that this is no longer a blocker for either PR, though this is certainly something worth looking into given the implications outside of mpn.scorecard.

The one warning Seth mentioned was a tidy evaluation warning suggesting to use all_of() in the traceability matrix tests. This is addressed in e39b9ff.

Traceback of one of the core failures on new BP

The original error was:

# Cant open R/ dir during covr
Browse[1]> covr::package_coverage(pkg_source_path, type = "tests")
⠙ |   1      1 | results  
Error in file(con, "r") : cannot open the connection

When passing clean = FALSE, quiet = FALSE, we get a more informative error. This was the error that suggested it may be blueprint related to me, mainly given A) No core packages have been updated recently, and B) @kyleam did not run into these errors in his recent PR extending support to eternal software.

Browse[1]> covr::package_coverage(pkg_source_path, type = "tests",  clean = FALSE, quiet = FALSE)
ERROR: dependencycheckmateis not available for packagepackage1* removing/tmp/Rtmp2zGIJM/R_LIBS2cd4d03ce4750a/package1’
⠦ |   16      1 | results    
Error in file(con, "r") : cannot open the connection

Full traceback

Browse[1]> package1_0.0.0.9000.covr
$name
[1] "package1_0.0.0.9000"

$coverage
$coverage$filecoverage
[1] NA

$coverage$totalcoverage
[1] NA


$errors
<scorecard_covr_error/callr_error/rlib_error_3_0/rlib_error/error>
  Error: 
  ! in callr subprocess.
Caused by error in `file(con, "r")`:
  ! cannot open the connection
---
  Standard error:
  ERROR: dependencycheckmateis not available for packagepackage1* removing/tmp/RtmpfLYyEp/R_LIBS2d73da476e9443/package1Warning messages:
  1: In utils::install.packages(repos = NULL, lib = install_path, pkg$path,  :
    installation of package/data/Projects/package_dev/mpn.scorecard/inst/testing_dir/package1had non-zero exit status
  2: In file(con, "r") :
    cannot open file '/tmp/RtmpfLYyEp/R_LIBS2d73da476e9443/package1/R/package1': No such file or directory
  ---
    Subprocess backtrace:
    1. covr::coverage_to_list(covr::package_coverage(p, type = "tests", …
    2. covr::tally_coverage(x) at covr.R:555:3
    3. base::as.data.frame(x) at summary_functions.R:24:3
    4. covr::package_coverage(p, type = "tests", ...) at covr.R:555:3
    5. covr:::add_hooks(pkg$package, install_path, fix_mcexit = should_enable_parallel_mcexit_fix(pkg)) at covr.R:457:3
    6. base::readLines(file.path(lib, pkg_name, "R", pkg_name)) at covr.R:772:3
    7. base::file(con, "r")
    8. base::.handleSimpleError(function (e) …
    9. global h(simpleError(msg, call))
    
    $notes
    [1] NA

@seth127
Copy link
Collaborator

seth127 commented Sep 10, 2024

Update on the test flakiness that @barrettk was investigating. I had seen this too (see my earlier comment) but when I cloned the repo fresh (and re-ran the renv::init(bare = T) and pkgr install) I didn't see the errors anymore. I tried on a few different configurations of Metworx blueprints and R versions and could never reproduce the errors.

All that to say I think these failures were just artifacts of both of us having slightly wonky dev set ups at the time. My guess is that the child sessions created by covr were sneaking into a different R version than the main session, and the packages weren't installed for that version.

Anyway, I'm definitely not considering that a blocker anymore. I'm giving this one final review now, but I expect to approve this and #73 soon.

@kyleam
Copy link
Collaborator

kyleam commented Sep 10, 2024

his recent PR extending support to eternal software

"software that stands the test of time" :)

Copy link
Collaborator

@seth127 seth127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I spot-checked a few more things (including pkgr and bbi, using the new feature from #70 ) and it all looks good.

I had one little comment about a code comment, but I don't feel too strongly about needing to change that here. At your discretion. Thanks for all of these updates.

@barrettk barrettk merged commit 0fba81a into main Sep 10, 2024
5 checks passed
@barrettk barrettk deleted the patch/trac-mat branch September 10, 2024 19:15
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.

3 participants