Skip to content

Permit perf test duration to be set, seconds, by environment variable…#660

Merged
StuartWheater merged 2 commits intodatashield:v6.3.6-devfrom
StuartWheater:v6.3.6-dev_feat-perf-support
Apr 14, 2026
Merged

Permit perf test duration to be set, seconds, by environment variable…#660
StuartWheater merged 2 commits intodatashield:v6.3.6-devfrom
StuartWheater:v6.3.6-dev_feat-perf-support

Conversation

@StuartWheater
Copy link
Copy Markdown
Member

Can use environment variable 'PERF_DURATION_SEC', in seconds, to specify individual test's duration.

Copy link
Copy Markdown
Contributor

@villegar villegar left a comment

Choose a reason for hiding this comment

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

These changes look sensible to me! One question, may for the future: where does the function perf.reference.save write the outputs? I know we are currently ignoring these tests on CRAN, but they don't like you writing files to non-temp directories, so we might want to be careful.

Copy link
Copy Markdown
Contributor

@timcadman timcadman left a comment

Choose a reason for hiding this comment

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

How about adding

ds.test_env$perf_duration_sec <- as.integer(Sys.getenv("PERF_DURATION_SEC", unset = "30"))

to setup.R and then calling in each performance file with ds.test_env$perf_duration_sec.

That way you wouldn't need to repeat that line in each performance test, which would be nice given that I'm planning to write performance tests for all the functions!

@StuartWheater
Copy link
Copy Markdown
Member Author

perf.reference.save used to write to the specified perf profile file if a value for a profile was missing. But the CRAN didn't approve of change files in the repo, so it currently write it to a file, temporary directory.

@StuartWheater
Copy link
Copy Markdown
Member Author

Some methods don't have a default of 30 seconds, e.g. on for connect-disconnect and void.

@timcadman
Copy link
Copy Markdown
Contributor

Some methods don't have a default of 30 seconds, e.g. on for connect-disconnect and void.

Could we set 2-3 environment variables? Just trying to avoid a way of repeating the same line of code 100 times. Need to finish today but can have a think tomorrow (long train journey)

@StuartWheater
Copy link
Copy Markdown
Member Author

We could have a method "perfDuration(unset)", but sometimes it feels clearer not to totally hide what is going on. As the test wouldn't be obviously linked to the value of "PERF_DURATION_SEC" environment variable.

@timcadman
Copy link
Copy Markdown
Contributor

Yes I would be inclined to go for something like:

 perf_duration <- function(default = 30) {                                                                                                                             
    base::as.integer(base::Sys.getenv("PERF_DURATION_SEC", unset = as.character(default)))
  }  

Then in each test you can just do:
.durationSec <- perf_duration()

Or
.durationSec <- perf_duration(60)

@StuartWheater StuartWheater requested a review from timcadman April 14, 2026 09:48
# context("ds.abs::perf:0")
test_that("combine - performance", {
.durationSec <- 30 # seconds
.durationSec <- perf.testduration(30)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that you have set the default as 30, you could rely on the default, ieperf.testduration(). But maybe you've done it like this to make it explicit. I'll approve the PR, and leave it up to you if you want to change or not.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My preference would be that a person reviewing the code would have an indication that the duration was alterable.

@StuartWheater StuartWheater merged commit 2cbe807 into datashield:v6.3.6-dev Apr 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants