Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 344 tests #385

Merged
merged 9 commits into from
Jan 3, 2022
Merged

Issue 344 tests #385

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
63ca7da
test-checkInterrupts.R check #244 test timing
zozlak Dec 21, 2021
5792b00
test-checkInterrupts.R: check if request has been cancelled sercer side
zozlak Dec 21, 2021
afa8616
test-checkInterrupts.R: restore the "query < 1 second" test
zozlak Dec 28, 2021
89047c5
Merge branch 'main' into issue-344-tests
krlmlr Dec 28, 2021
8c4acaf
Can't update snapshots in foreign pull requests
krlmlr Dec 28, 2021
8eb097b
Update snapshots also for dev
krlmlr Dec 28, 2021
a8db64d
Merge branch 'main' into issue-344-tests
krlmlr Dec 28, 2021
fc1d35c
Try to shorten the checked time to account for different timer resolu…
krlmlr Dec 28, 2021
a3a29d1
Merge branch 'main' into issue-344-tests
krlmlr Jan 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions tests/testthat/test-checkInterrupts.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
context("checkInterrupts")

test_that("check_interrupts = TRUE works with queries < 1 second (#244)", {
test_that("check_interrupts = TRUE works with queries < 1 second", {
con <- postgresDefault(check_interrupts = TRUE)
time <- system.time(
expect_equal(dbGetQuery(con, "SELECT pg_sleep(0.2), 'foo' AS x")$x, "foo")
Expand All @@ -9,6 +9,15 @@ test_that("check_interrupts = TRUE works with queries < 1 second (#244)", {
dbDisconnect(con)
})

test_that("check_interrupts = TRUE works with queries > 1 second (#244)", {
Copy link
Member

Choose a reason for hiding this comment

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

The original test is good. Can you please create a new test?

Should the test condition be expect_lt(time[["elapsed"]], 0.9) then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we claim this is a test for #244, it should definitely test a query longer than 1 second because the #244 was caused by a fact that the structure taken by the select() requires different reinitialization under linux and windows, after the select() timeout is reached (and we hardcode the timeout to 1 second).

Of course I can also add another test with query shorter than 1 second, just then we shouldn't claim it tests #244.

Copy link
Member

@krlmlr krlmlr Dec 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe the reference to #244 is wrong, I'd need to take a closer look. Either way, can you please keep the original test?

Copy link
Member

Choose a reason for hiding this comment

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

The new test is failing, btw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, restored the old removing the reference to #244 and added the > 1 second one with the reference to #244 and fixed check condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, the test CI failed on the "update snapshots" step - I have no idea what does it mean.
When I run tests locally all pass but (test-checkInterrupts.R:50:3): check_interrupts = TRUE interrupts immediately (#336) (which means the query isn't cancelled on the server side when the interrupt breaks dbGetQuery()).

con <- postgresDefault(check_interrupts = TRUE)
time <- system.time(
expect_equal(dbGetQuery(con, "SELECT pg_sleep(2), 'foo' AS x")$x, "foo")
)
expect_gt(time[["elapsed"]], 1.5)
dbDisconnect(con)
})

test_that("check_interrupts = TRUE interrupts immediately (#336)", {
skip_if_not(postgresHasDefault())
skip_if(Sys.getenv("R_COVR") != "")
Expand All @@ -20,6 +29,8 @@ test_that("check_interrupts = TRUE interrupts immediately (#336)", {
session$run(function() {
library(RPostgres)
.GlobalEnv$conn <- postgresDefault(check_interrupts = TRUE)
.GlobalEnv$connPid <- dbGetQuery(.GlobalEnv$conn, "SELECT pg_backend_pid() AS pid")$pid
.GlobalEnv$checkConn <- postgresDefault()
invisible()
})

Expand All @@ -31,8 +42,13 @@ test_that("check_interrupts = TRUE interrupts immediately (#336)", {
session$interrupt()
# Interrupts are slow on Windows, give ample time
expect_equal(session$poll_process(2000), "ready")
session$read()

queryStatus = session$run(function() {
dbGetQuery(.GlobalEnv$checkConn, "SELECT state FROM pg_stat_activity WHERE pid = $1", params = .GlobalEnv$connPid)
})
expect_equal(queryStatus$state, "idle")

# Tests for error behavior are brittle

session$close()
})