force datetime creation. closes #685

[JanMarvin]

Not sure if this cures #685. Could you have a look @jmbarbone ? Have to say this wrapper test function is still a bit confusing to me.

[jmbarbone]

I feel like there could be something funny here:

openxlsx2/R/class-workbook.R

Lines 183 to 193 in 757a6b5

	initialize = function(
	creator = NULL,
	title = NULL,
	subject = NULL,
	category = NULL,
	datetime_created = Sys.time(),
	theme = NULL,
	...
	) {
	standardize_case_names(...)

The datetime_created field may not be evaluated right away. The standardize_case_names() could cause a bit of a delay.

foo <- function(x = Sys.time()) {
  print(Sys.time())
  Sys.sleep(2)
  x
}

foo()
#> [1] "2023-07-13 10:25:30 EDT"
#> [1] "2023-07-13 10:25:32 EDT"

^{Created on 2023-07-13 with reprex v2.0.2}

I haven't looked at standardize_case_names() much so I don't know what would happen when we detect camel case in ... for an argument outside of ....

[JanMarvin]

Thanks, interesting finding. Maybe we can force Sys.time(). I assume it's just a matter of microseconds rounding in one or the other way.

[JanMarvin]

I have added force() for the datetime, but maybe this field should simply be skipped. The datetime string does not matter in this test, even if the function was called years later, it wouldn't matter as long as the arguments match.

The current CRAN error is without standardize_case_names(...). So it's either something else or bad luck.

[JanMarvin]

Hmpf. No love for Sys.time().

== Failed tests ================================================================
-- Failure ('test-class-workbook-wrappers.R:422:3'): wb_add_thread() is a wrapper --
wbWorkbook$add_thread$threadComments[[1]] vs wb_add_thread$threadComments[[1]]
- "<threadedComment ref=\"A1\" dT=\"2023-07-20T05:14:23Z\" personId=\"{547E1F9B-459E-2B92-2C65-9044D4BEF1C4}\" id=\"{547E1F9B-459E-2B92-2C65-9044D4BEF1C4}\" done=\"0\"><text>test</text></threadedComment>"
+ "<threadedComment ref=\"A1\" dT=\"2023-07-20T05:14:22Z\" personId=\"{547E1F9B-459E-2B92-2C65-9044D4BEF1C4}\" id=\"{547E1F9B-459E-2B92-2C65-9044D4BEF1C4}\" done=\"0\"><text>test</text></threadedComment>"

[ FAIL 1 | WARN 0 | SKIP 52 | PASS 1149 ]

[jmbarbone]

Just adding a reprex of the inconsistency:

library(openxlsx2)
library(testthat)

# to load in expect_wrapper()
source("/home/jordan/github/openxlsx2/tests/testthat/helper.R")

for (i in 1:200) {
  res <- tryCatch({
    wb <- wb_workbook()$add_worksheet()
    
    expect_wrapper(
      "add_person",
      wb = wb,
      params = list(name = "me")
    )
    
    wb <- wb_workbook()$add_worksheet()$add_person("me")
    
    expect_wrapper(
      "get_person",
      wb = wb,
      params = list(name = "me")
    )
    
    wb <- wb_workbook()$add_worksheet()$add_person("me")
    me_id <- wb$get_person("me")$id
    
    expect_wrapper(
      "add_thread",
      wb = wb,
      params = list(comment = "test", person_id = me_id)
    )
  }, 
  expectation_failure = function(e) {
    print(e)
    "break"
  })
  
  if (identical(res, "break")) break
}
#> <expectation_failure/expectation/error/condition>
#> wbWorkbook$add_thread$threadComments[[1]] vs wb_add_thread$threadComments[[1]]
#> - "<threadedComment ref=\"A1\" dT=\"2023-07-20T15:45:14Z\" personId=\"{9290C301-459E-2B92-2C65-9044D4BEF1C4}\" id=\"{9290C301-459E-2B92-2C65-9044D4BEF1C4}\" done=\"0\"><text>test</text></threadedComment>"
#> + "<threadedComment ref=\"A1\" dT=\"2023-07-20T15:45:13Z\" personId=\"{9290C301-459E-2B92-2C65-9044D4BEF1C4}\" id=\"{9290C301-459E-2B92-2C65-9044D4BEF1C4}\" done=\"0\"><text>test</text></threadedComment>"
#> Backtrace:
#>     ▆
#>  1. ├─base::tryCatch(...)
#>  2. │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  3. │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  4. │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
#>  5. └─global expect_wrapper(...)

print(i)
#> [1] 80

^{Created on 2023-07-20 with reprex v2.0.2}

[JanMarvin]

Thanks for the reprex!

I still think the issue is that we do not provide a single timestamp for either the workbook creation or the comment creation. Since R is single threaded, functions will run in sequential order and since time is linear, chances are that we will see for the same function timestamps $$x_{t}$$ and $$x_{t+1}.$$ The question remains, however, why the error has been occurring lately, but it could simply be that the wbWorkbook object has been getting bigger and now the runtime has slowed down a bit - perhaps the parallel tests have covered that up - but at least it's slow enough now that the odds are simply against us now. At least we manage several hundred runs without any errors. (The reprex ran without any failure for me).

One solution might be to pass the creation time to the function (oddly, we didn't do that before) or to skip fields that contain creation time strings (file creation date and thread comment date might be the only ones). I don't think there's a real problem, it's just that the test simply compares strings and one of the strings shows a deviation of 1 second and boom.

[JanMarvin]

This @#$%&! still bugs us. Now on CRANs r-patched.

 ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-class-workbook-wrappers.R:454:3'): wb_add_thread() is a wrapper ──
  wbWorkbook$add_thread$threadComments[[1]] vs wb_add_thread$threadComments[[1]]
  - "<threadedComment ref=\"A1\" dT=\"2023-11-22T22:53:55Z\" personId=\"{460E9460-459E-2B92-2C65-9044D4BEF1C4}\" id=\"{460E9460-459E-2B92-2C65-9044D4BEF1C4}\" done=\"0\"><text>test</text></threadedComment>"
  + "<threadedComment ref=\"A1\" dT=\"2023-11-22T22:53:54Z\" personId=\"{460E9460-459E-2B92-2C65-9044D4BEF1C4}\" id=\"{460E9460-459E-2B92-2C65-9044D4BEF1C4}\" done=\"0\"><text>test</text></threadedComment>"
  
  [ FAIL 1 | WARN 0 | SKIP 61 | PASS 1301 ]
  Error: Test failures
  Execution halted

We set the date in options("openxlsx2.datetimeCreated" = as.POSIXct("2023-07-20 23:32:14", tz = "UTC")) in test-aaa.R and this does not work. Apparently we have to set the option on top of every test file.

[Edit] Next attempt to fix this is here: #857