Documentation: Clean up unit testting suite for date/datetime stuff

[zdz2101]

Please select a category the issue is focused on?

Other

Let us know where something needs a refresh or put your idea here!

For each of the 3 test files: test-derive_vars_dt_dtm_utils.R , test-derive_vars_dt.R , and test_derive_vars_dtm.R there may be missing tests or additional coverage like NULL, NA, edge cases that need to be flushed out to make development easier.

Task will be broken into multiple parts (feel free to turn each bullet into an issue and work on it):

In test-derive_vars_dt_dtm_utils.R:

• General Issue: Add unit test for dt_level() #2377
• General Issue: Add unit test for get_imputation_target_date() #2378
• Add unit test for get_imputation_target_time() #2409

In test-derive_vars_dt.R:

There is some merit to consistently keep using the same input dataset/thing to test, where we can do:

• complete date
• missing year
• missing month & day
• missing day
• missing only month
• empty string
• NA_character_
• inappropiate string format

and apply to all the above^. For the most part I think we do an appropriate range of tests, but can certainly do a better job at the test description or adding code comments where it is appropriate to explain the use case.

Cleanup/make appropriate additions for:

• Create united example for test suite of derive_vars_dt() #2400
• look into withr to remove use of global variables in the unified example
• cleanup compute_dtf(): closed per Clean up documentation on datetime/date flagging functions, e.g. compute_dtf()/compute_tmf() to make development/maintenance easier #2343
• add unit test for restricted_imputed_dtc_dt() #2495
• cleanup impute_dtc_dt(): closed with Create united example for test suite of derive_vars_dt() #2400
• Using the unified example apply to convert_dtc_to_dt() as necessary and figure out removal of global environment as well #2424
• cleanup derive_vars_dt()

In test-derive_vars_dtm.R:

same as above but add:

• complete time
• missing hour
• missing min
• missing sec

Cleanup/make appropriate additions for:

• cleanup compute_tmf(): closed per Clean up documentation on datetime/date flagging functions, e.g. compute_dtf()/compute_tmf() to make development/maintenance easier #2343
• add unit test for restricted_imputed_dtc_dtm()
• cleanup impute_dtc_dtm()
• cleanup convert_dtc_to_dtm()
• cleanup derive_vars_dtm()

Dreaming...

• Create developer-focused flow diagram of how are dt/dtm functions work

[zdz2101]

@bms63 @millerg23 @StefanThoma think I drew up what is appropriate to do unit-testing wise, any thoughts before I churn them into individual bite sized issues such that we can get definitions of done? What're peoples' thoughts on snapshot testing as well?

[bms63]

I really like the idea of having the same data object that we use throughout the entire date/time testing suite!!

Can we construct that first before we start updating any tests?

[StefanThoma]

Very happy for snapshot tests.
@bms63 what did you have in mind?

[millerg23]

Regarding snapshot testing, is this for ERROR checking? We are implementing snapshot testing in admiraldev for assertions, this is part of the updates to ERROR messaging. So only need to snapshot the unit test in admiraldev, we should just be able to test for class of ERROR in admiral, or test partial strings.

[bms63]

Very happy for snapshot tests. @bms63 what did you have in mind?

@StefanThoma - From Zelos' proposal. I was thinking the data object would have all these and anything else needed for other date/time tests. Then this object is used throughout all the tests and if we update the object, for some new issue, it runs through every single test again.

There is some merit to consistently keep using the same input dataset/thing to test, where we can do:

• complete date
• missing year
• missing month & day
• missing day
• missing only month
• empty string
• NA_character_
• inappropiate string format

[zdz2101]

Very happy for snapshot tests. @bms63 what did you have in mind?

@StefanThoma - From Zelos' proposal. I was thinking the data object would have all these and anything else needed for other date/time tests. Then this object is used throughout all the tests and if we update the object, for some new issue, it runs through every single test again.

There is some merit to consistently keep using the same input dataset/thing to test, where we can do:

• complete date
• missing year
• missing month & day
• missing day
• missing only month
• empty string
• NA_character_
• inappropiate string format

Precisely, @millerg23 as for why snapshots, on top of the errors, I think it's also just easier to review once we have a known example that we use consistently, don't think we need to remove the expected_output and expect_equal() statements but it's a nice touch, and if something changes, it'll be easier to spot in the snapshot

[bms63]

@StefanThoma @millerg23 @zdz2101 I follow this guy on LinkedIn and he is a goldmine. The Unit Testing principles are for Java, but I thought it was still a cool read - https://docs.google.com/document/d/1_Z9CjOjynrmZiYIvGqrR5sZ2s7v1f-RePAjdFHy9XEk/mobilebasic

[bms63]

@zdz2101 could we make this into a discussion? Does the click on issue creation happen when this is in a discussion?

[zdz2101]

Didn't know until we tried, does indeed lose that feature, but its okay, the old school way is fine too and follows the issue templates more closely/appropriately