Run integration tests against many versions; remove warnings unless outside of that range #244

nealrichardson · 2024-04-17T20:44:53Z

Edits integration-tests.yaml and supporting machinery to run against a matrix of versions. I went back to the oldest tagged version of our docker images, 1.8.8.2 from May 2021, and otherwise went every 6 months, from 2021.09.0 to 2024.03.0, the latest. Only very minor changes to some tests were required to be flexible for a few places where we've added fields to the API responses over the years.

Since it appears that the package generally works fine across this matrix of versions, I removed some of the scary language and warnings about Connect-connectapi version mismatch, and instead only warn if your Connect server is older than 1.8.8.2.

Some light refactoring in a few places I had to touch along the way too.

Fixes #230, #105

nealrichardson · 2024-04-17T21:15:46Z

Current failures across the matrix of Connect versions. Looks like there are a very small number of (tested) API changes we need to either adapt the package code to handle (or block the actions on too old versions), or skip tests conditionally.

Older versions

1.8.8.2

Failure ('test-get.R:57:3'): get_audit_logs works
vctrs::vec_ptype(audit_list) not equal to vctrs::vec_ptype(connectapi_ptypes$audit_logs).
Names: 4 string mismatches

Failure ('test-lazy.R:86:3'): content works
vctrs::vec_ptype(content_list_local) not equal to vctrs::vec_ptype(connectapi_ptypes$content).
Names: 12 string mismatches
Component 23: Modes: logical, character
Component 23: target is logical, current is character
Component 26: Modes: character, logical
Component 26: target is character, current is logical

Failure ('test-lazy.R:116:3'): audit_logs works
vctrs::vec_ptype(audit_list_local) not equal to vctrs::vec_ptype(connectapi_ptypes$audit_logs).
Names: 4 string mismatches

[ FAIL 3 | WARN 1 | SKIP 26 | PASS 417 ]

2021.09.0

Basically the same:

Failure ('test-get.R:57:3'): get_audit_logs works
vctrs::vec_ptype(audit_list) not equal to vctrs::vec_ptype(connectapi_ptypes$audit_logs).
Names: 4 string mismatches

Failure ('test-lazy.R:86:3'): content works
vctrs::vec_ptype(content_list_local) not equal to vctrs::vec_ptype(connectapi_ptypes$content).
Names: 13 string mismatches
Component 24: Modes: logical, character
Component 24: target is logical, current is character
Component 26: Modes: character, logical
Component 26: target is character, current is logical
Component 30: Modes: list, character
Component 32: Modes: character, list
Component 32: target is character, current is list

Failure ('test-lazy.R:116:3'): audit_logs works
vctrs::vec_ptype(audit_list_local) not equal to vctrs::vec_ptype(connectapi_ptypes$audit_logs).
Names: 4 string mismatches

[ FAIL 3 | WARN 1 | SKIP 26 | PASS 417 ]

2022.03.2

Failure ('test-get.R:57:3'): get_audit_logs works
vctrs::vec_ptype(audit_list) not equal to vctrs::vec_ptype(connectapi_ptypes$audit_logs).
Names: 4 string mismatches

Failure ('test-lazy.R:116:3'): audit_logs works
vctrs::vec_ptype(audit_list_local) not equal to vctrs::vec_ptype(connectapi_ptypes$audit_logs).
Names: 4 string mismatches

[ FAIL 2 | WARN 1 | SKIP 26 | PASS 418 ]

Current version: 2022.09.0

This is surprising because the only difference seems to be the change in base image from bionic to jammy. We may want to switch some of these versions to use the bionic tag since the issue seems to go away by 2023.09.

Error ('test-content.R:86:1'): (code run outside of `test_that()`)
Error in `poll_task(tsk_rmd)`: An error occurred while running your content. (Error code: render-r-code-error)
Backtrace:
    ▆
 1. ├─base::suppressMessages(poll_task(tsk_rmd)) at test-content.R:86:1
 2. │ └─base::withCallingHandlers(...)
 3. └─connectapi::poll_task(tsk_rmd)

Error ('test-schedule.R:22:1'): (code run outside of `test_that()`)
Error in `poll_task(tsk_rmd)`: An error occurred while running your content. (Error code: render-r-code-error)
Backtrace:
    ▆
 1. ├─base::suppressMessages(poll_task(tsk_rmd)) at test-schedule.R:22:1
 2. │ └─base::withCallingHandlers(...)
 3. └─connectapi::poll_task(tsk_rmd)

[ FAIL 2 | WARN 0 | SKIP 21 | PASS 263 ]

Newer versions

2023.03.0

Error ('test-content.R:86:1'): (code run outside of `test_that()`)
Error in `poll_task(tsk_rmd)`: An error occurred while running your content. (Error code: render-r-code-error)
Backtrace:
    ▆
 1. ├─base::suppressMessages(poll_task(tsk_rmd)) at test-content.R:86:1
 2. │ └─base::withCallingHandlers(...)
 3. └─connectapi::poll_task(tsk_rmd)

Failure ('test-deploy.R:439:3'): deployment timestamps respect timezone
any(...) is not TRUE

`actual`:   FALSE
`expected`: TRUE 

Error ('test-schedule.R:22:1'): (code run outside of `test_that()`)
Error in `poll_task(tsk_rmd)`: An error occurred while running your content. (Error code: render-r-code-error)
Backtrace:
    ▆
 1. ├─base::suppressMessages(poll_task(tsk_rmd)) at test-schedule.R:22:1
 2. │ └─base::withCallingHandlers(...)
 3. └─connectapi::poll_task(tsk_rmd)

[ FAIL 3 | WARN 1 | SKIP 21 | PASS 262 ]

2023.09.0

Failure ('test-deploy.R:439:3'): deployment timestamps respect timezone
any(...) is not TRUE

`actual`:   FALSE
`expected`: TRUE 

Failure ('test-lazy.R:86:3'): content works
vctrs::vec_ptype(content_list_local) not equal to vctrs::vec_ptype(connectapi_ptypes$content).
Names: 19 string mismatches
Length mismatch: comparison on first 32 components
Component 14: Modes: logical, numeric
Component 14: Attributes: < Length mismatch: comparison on first 1 components >
Component 14: Attributes: < Component "class": Lengths (1, 2) differ (string compare on first 1) >
Component 14: Attributes: < Component "class": 1 string mismatch >
Component 14: target is vctrs_unspecified, current is POSIXct
Component 15: Modes: logical, numeric
Component 15: Attributes: < Length mismatch: comparison on first 1 components >
...

[ FAIL 2 | WARN 1 | SKIP 26 | PASS 418 ]

2024.03.0

Failure ('test-deploy.R:439:3'): deployment timestamps respect timezone
any(...) is not TRUE

`actual`:   FALSE
`expected`: TRUE 

Failure ('test-get.R:50:3'): get_usage_static works
vctrs::vec_ptype(content_visits) not equal to vctrs::vec_ptype(connectapi_ptypes$usage_static).
Names: 1 string mismatch
Length mismatch: comparison on first 7 components
Component 7: Modes: character, numeric
Component 7: target is character, current is numeric

Failure ('test-lazy.R:57:3'): usage_static works
vctrs::vec_ptype(content_visits_local) not equal to vctrs::vec_ptype(connectapi_ptypes$usage_static).
Names: 1 string mismatch
Length mismatch: comparison on first 7 components
Component 7: Modes: character, numeric
Component 7: target is character, current is numeric

Failure ('test-lazy.R:86:3'): content works
vctrs::vec_ptype(content_list_local) not equal to vctrs::vec_ptype(connectapi_ptypes$content).
Names: 19 string mismatches
Length mismatch: comparison on first 32 components
Component 14: Modes: logical, numeric
Component 14: Attributes: < Length mismatch: comparison on first 1 components >
Component 14: Attributes: < Component "class": Lengths (1, 2) differ (string compare on first 1) >
Component 14: Attributes: < Component "class": 1 string mismatch >
Component 14: target is vctrs_unspecified, current is POSIXct
Component 15: Modes: logical, numeric
Component 15: Attributes: < Length mismatch: comparison on first 1 components >
...

[ FAIL 4 | WARN 1 | SKIP 26 | PASS 416 ]

nealrichardson · 2024-04-17T23:57:13Z

I've fixed 2 of the jobs now. The remaining issues all have to do with strict type checking of a couple of API responses that appear to have gained attributes over time. I'll see if I can easily adapt the code to handle this, but if it's a more invasive change than I want to get into here, I may just conditionally skip the tests and create followup issues. It's a more general need to make the package more resilient to evolution in the Connect API, and that probably exceeds the scope of this PR.

jonkeane

Thanks for this. Both suggestions are very minor

.github/local/test-connect-ci.yml

.github/workflows/integration-tests.yaml

Makefile

jonkeane · 2024-04-18T13:19:56Z

R/utils-ci.R

+version_to_docker_tag <- function(version) {
+  # Prior to 2022.09.0, the plain version number was the tag
+  # After, it's "<ubuntu-codename>-<version>"
+  # If you want a specific image tag, just pass it in and it will go through unchanged
+  try(
+    {
+      numver <- numeric_version(version)
+      if (numver > "2023.06") {
+        version <- paste0("jammy-", version)
+      } else if (numver >= "2022.09") {
+        # There's both jammy and bionic for these, but the bionic ones
+        # seem more reliable in CI
+        version <- paste0("bionic-", version)
+      }
+    },
+    silent = TRUE
+  )
+  version
+}
+


At first I thought this might be over-complicated, but it's good, and we will soon have noble- to deal with too.

This is really just sugar; if you know the image tag you can just specify it (now, that is, bionic was hard-coded before). This just prevents us from having to remember whether it's bionic or jammy or whatever else.

tests/integrated/test-deploy.R

jonkeane · 2024-04-18T13:25:55Z

tests/integrated/test-deploy.R


  # we just did this, so it should be less than 1 minute ago...
  # (really protecting against being off by hours b/c of timezone differences)
-  expect_true(any((Sys.time() - allusg$time) < lubridate::make_difftime(60, "seconds")))
+  expect_lt(Sys.time() - allusg$time, lubridate::make_difftime(60, "seconds"))


Suggested change

expect_lt(Sys.time() - allusg$time, lubridate::make_difftime(60, "seconds"))

expect_lt(Sys.time() - allusg$time, lubridate::make_difftime(60, "seconds"), label = "something is wrong with timezones since: object")

This is very hacky, but would at least get that this is actually a timezone check a little bit clearer? Or maybe there's a better timezone check we should do more directly with the output?

Eh, I think this will be better enough

…versions

…ure is clearer if it happens

…unless older than the oldest we now test

jonkeane approved these changes Apr 18, 2024

View reviewed changes

nealrichardson changed the title ~~Update Connect version tested against, and test a matrix of versions~~ Run integration tests against many versions; remove warnings unless outside of that range Apr 18, 2024

nealrichardson marked this pull request as ready for review April 18, 2024 20:35

nealrichardson linked an issue Apr 18, 2024 that may be closed by this pull request

Create test matrix for different Connect versions #105

Closed

nealrichardson added 11 commits April 18, 2024 17:00

s/RSC_VERSION/CONNECT_VERSION/g

5dc251f

s/rsc_version/connect_version/g

fc0f16d

Adapt script and docker-compose to be able to take different Connect …

c5cba2e

…versions

Test matrix of connect versions, why not?

f836066

Try using bionic images in some places

f19ac55

Add some sleeping to usage stat test, and refactor assertions so fail…

566d8df

…ure is clearer if it happens

Slight refactor and try to fix remaining tests

6a299f1

🤦

6cf432d

s/allusg/all_usage/g

c181073

Bump default version to 2024.03.0; don't warn about version mismatch …

c4c1dd3

…unless older than the oldest we now test

Add NEWS

cde9e21

nealrichardson force-pushed the use-newer-connect branch from f3e6ebf to cde9e21 Compare April 18, 2024 21:00

nealrichardson merged commit 3ec8489 into main Apr 18, 2024
19 checks passed

nealrichardson deleted the use-newer-connect branch April 18, 2024 21:11

fh-mthomson mentioned this pull request May 22, 2024

Update version of Connect used to test common workflows (e.g., git-backed deployment) #205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run integration tests against many versions; remove warnings unless outside of that range #244

Run integration tests against many versions; remove warnings unless outside of that range #244

nealrichardson commented Apr 17, 2024 •

edited

Loading

nealrichardson commented Apr 17, 2024

nealrichardson commented Apr 17, 2024

jonkeane left a comment

jonkeane Apr 18, 2024

nealrichardson Apr 18, 2024

jonkeane Apr 18, 2024

nealrichardson Apr 18, 2024

	expect_lt(Sys.time() - allusg$time, lubridate::make_difftime(60, "seconds"))
	expect_lt(Sys.time() - allusg$time, lubridate::make_difftime(60, "seconds"), label = "something is wrong with timezones since: object")

Run integration tests against many versions; remove warnings unless outside of that range #244

Run integration tests against many versions; remove warnings unless outside of that range #244

Conversation

nealrichardson commented Apr 17, 2024 • edited Loading

nealrichardson commented Apr 17, 2024

Older versions

1.8.8.2

2021.09.0

2022.03.2

Current version: 2022.09.0

Newer versions

2023.03.0

2023.09.0

2024.03.0

nealrichardson commented Apr 17, 2024

jonkeane left a comment

Choose a reason for hiding this comment

jonkeane Apr 18, 2024

Choose a reason for hiding this comment

nealrichardson Apr 18, 2024

Choose a reason for hiding this comment

jonkeane Apr 18, 2024

Choose a reason for hiding this comment

nealrichardson Apr 18, 2024

Choose a reason for hiding this comment

nealrichardson commented Apr 17, 2024 •

edited

Loading