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

getDbCohortMethodData unit-tests #146

Merged
merged 13 commits into from
Aug 28, 2023
Merged

Conversation

mvankessel-EMC
Copy link
Collaborator

I've added dedicated unit-tests for getDbCohortMethodData(). Each parameter of getDbCohortMethodDate() has a dedicated set of tests.

I've also made the following changes in the existing code:

  1. studyStartDate/studyEndDate
    a. Added a function dateCheck() to check if a date is valid to replace the regex approach. The old regex approach allowed for non-existing dates like "20231736" (2023-17-36).
    b. Removed the if (is.null()) checks, as studyStartDate and studyEndDate will never equal NULL, because of the checkmate assertion.
  2. cdmVersion
    Updated the assertion on cdmVersion, the old check allowed for any character of any length.
  3. on.exit() in uploadExportedResults()
    Wrapped the unlink() and disconnect() calls in withr::defer() and tryCatch(), as the following error kept showing up on Ubuntu when running R-CMD-Check in the actions, when running test-eunomia.R:
Error in `uploadExportedResults(connectionDetails = connectionDetails, 
      databaseSchema = "main", append = append, exportFolder = exportFolder, 
      cohorts = cohorts)`: object 'databaseIdentifierFile' not found

and

Error in `uploadExportedResults(connectionDetails = connectionDetails, 
    databaseSchema = "main", append = append, exportFolder = exportFolder, 
    cohorts = cohorts)`: object 'connection' not found

I suspect that both connection and databaseIdentifierFile are already cleaned up before unlink(databaseIdentifierFile) and disconnect(connection) get to actually run on Ubuntu. I've tried playing around with on.exit(add, after) as well, but that didn't seem to be working either.

@mvankessel-EMC mvankessel-EMC self-assigned this Aug 24, 2023
@mvankessel-EMC mvankessel-EMC changed the base branch from main to develop August 24, 2023 07:56
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #146 (ad4c7ac) into develop (fb43f26) will increase coverage by 0.22%.
Report is 18 commits behind head on develop.
The diff coverage is 91.30%.

❗ Current head ad4c7ac differs from pull request most recent head 56c3647. Consider uploading reports for the commit 56c3647 to get more accurate results

@@             Coverage Diff             @@
##           develop     #146      +/-   ##
===========================================
+ Coverage    88.10%   88.33%   +0.22%     
===========================================
  Files           23       23              
  Lines         5340     5356      +16     
===========================================
+ Hits          4705     4731      +26     
+ Misses         635      625      -10     
Files Changed Coverage Δ
R/Viewer.R 66.95% <80.00%> (+0.60%) ⬆️
R/DataLoadingSaving.R 97.78% <100.00%> (+5.01%) ⬆️

... and 1 file with indirect coverage changes

@schuemie
Copy link
Member

Thanks, this is excellent work! I'll do some more digging into the on.exit() weirdness. I've never seen that before.

@schuemie schuemie merged commit e1b07c6 into develop Aug 28, 2023
8 checks passed
@schuemie
Copy link
Member

PS. I think we'll deprecate the cdmVersion argument sooner rather than later. We should be able to get that from the cdm_source table.

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