Sage-Bionetworks / dccmonitor

Allows for monitoring of data uploaded via the dccvalidator application. Functions for getting information on the uploaded files, metadata validation status, and more.
https://sage-bionetworks.github.io/dccmonitor
Other
2 stars 4 forks source link

Fix GH Actions #100

Closed Aryllen closed 3 years ago

Aryllen commented 3 years ago

Fixes #99.

Changes:

Aryllen commented 3 years ago

These might still fail. I can run them individually and have them be fine, but the validate_study tests fail when using devtools::test(). For example, I can see that the test checking that validate_study returns a list actually succeeds. When running it line-by-line, there is some console output from readr that's coming from a dccvalidator function, I believe. This might be causing the test to fail. Will look more into this.

karawoo commented 3 years ago

I haven't been able to replicate these specific failures locally, but looking at the check results it seems like it's the get_most_recent_time() tests that are failing:

 > test_check("dccmonitor")
── 1. Failure: get_most_recent_time gets the most recent time (@test-utils.R#37)
`res2` not equal to ...[].
Attributes: < Modes: list, NULL >
Attributes: < Lengths: 1, 0 >
Attributes: < names for target but not for current >
Attributes: < current is not list-like >

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 95 | SKIPPED: 4 | WARNINGS: 0 | FAILED: 1 ]
1. Failure: get_most_recent_time gets the most recent time (@test-utils.R#37) 

I don't really know why these would fail, but I do think it might be possible to simplify these tests and that might help. You use as.POSIXct(strptime("2019-12-15 01:30:00", "%Y-%m-%d %H:%M:%S"), tz = "UTC") often, which converts the string to POSIXlt and then to POSIXct, where it might be more straightforward to create the POSIXct object directly (as.POSIXct("2019-12-15 01:30:00", tz = "UTC")). It also seems like the get_most_recent_time() function is basically max() -- unless there are some differences that I'm missing, it might be easier to use max() and remove these tests entirely.

Aryllen commented 3 years ago

You use as.POSIXct(strptime("2019-12-15 01:30:00", "%Y-%m-%d %H:%M:%S"), tz = "UTC") often, which converts the string to POSIXlt and then to POSIXct, where it might be more straightforward to create the POSIXct object directly (as.POSIXct("2019-12-15 01:30:00", tz = "UTC")).

I remember struggling with getting the time close to correct (it still isn't spot on compared to displayed Synapse times, but close enough). This is something I would like to improve. In general, I just needed it to give me a big number (time since epoch), which I probably just overcomplicated it thinking that I needed to pull these big numbers from time at all (testing with a vector of numbers). That added in with the issues of getting the right times in the app itself probably led to that mess of double time conversion. 🤦

It also seems like the get_most_recent_time() function is basically max() -- unless there are some differences that I'm missing, it might be easier to use max() and remove these tests entirely.

This is definitely max(). I don't remember why I wasn't getting max() to work with this. I'm going to just try switching to that. It should work and maybe it will be easier to figure out any problems now that I've had so much time away from the 'time' issue.

Thank you for checking this out!

Aryllen commented 3 years ago

Yay! It's working now. Merging.