Skip to content

fix: Ensure that testthat::skip() is only called in RSQLite's own tests, never in the tests for other packages - #516

Merged
krlmlr merged 8 commits into
r-dbi:mainfrom
jeanetteclark:b-515-testskips
Jul 18, 2024
Merged

fix: Ensure that testthat::skip() is only called in RSQLite's own tests, never in the tests for other packages#516
krlmlr merged 8 commits into
r-dbi:mainfrom
jeanetteclark:b-515-testskips

Conversation

@jeanetteclark

@jeanetteclark jeanetteclark commented Jul 12, 2024

Copy link
Copy Markdown
Contributor

Fixes #515

  • added setup.R to set package_name option and unset it after test are run
  • removed dbConnect from list of packages to check (AFAICT it doesn't exist anymore)
  • minor doc update that cropped up after running R CMD check

remaining tasks for #515

@aviator-app

aviator-app Bot commented Jul 12, 2024

Copy link
Copy Markdown
Contributor

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@krlmlr krlmlr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, good start. Would you like to add the rest of the implementation to this PR?

For the next steps: Inside if (), && is always better than & .

Comment thread NAMESPACE
import(DBI)
import(methods)
import(rlang)
importFrom(DBI,Id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting, where does this come from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I ran a CMD check after making my changes out of habit and it showed up on devtools::document(). figured I'd go ahead and commit it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note to self: this should have happened automatically, but hasn't due to "The decor package(s) are required for this functionality": https://github.com/r-dbi/RSQLite/actions/runs/9949313875/job/27485358644#step:11:75 .

Comment thread R/dbConnect_SQLiteDriver.R Outdated
Comment thread tests/testthat/setup.R Outdated
jeanetteclark and others added 2 commits July 15, 2024 10:04
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
@jeanetteclark

Copy link
Copy Markdown
Contributor Author

Would you like to add the rest of the implementation to this PR?

As discussed in #515, the rest of the changes have to go into dm. You mentioned that you would make the changes over there but I can also make the PR if you prefer, I think its just one line change. Let me know

@krlmlr krlmlr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. Happy to review everything we want to do here as a single PR here, I can easily move to dm then.

Comment thread NAMESPACE
import(DBI)
import(methods)
import(rlang)
importFrom(DBI,Id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note to self: this should have happened automatically, but hasn't due to "The decor package(s) are required for this functionality": https://github.com/r-dbi/RSQLite/actions/runs/9949313875/job/27485358644#step:11:75 .

also fix the formatting of the message

@krlmlr krlmlr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Can you please also update the header of the standalone file to include the changes and the new dependency?

Comment thread R/import-standalone-check_suggested.R Outdated
Comment thread R/import-standalone-check_suggested.R
@jeanetteclark

jeanetteclark commented Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

not sure why that test is failing, seems unrelated? here's the relevant log output:

  ── Failure ('test-dbConnect.R:231:3'): busy_handler timeout ────────────────────
  time < as.difftime(1, units = "secs") is not TRUE
  
  `actual`:   FALSE
  `expected`: TRUE 

@krlmlr krlmlr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the updates and for your patience. Should we do one more round?

There's a "Changelog" section in the header that could use a bullet.

This particular test is flaky, I need to review it at some point.

Comment thread R/import-standalone-check_suggested.R Outdated
# Skip if some packages are not installed when testing
# And say which package was not installed.
if (identical(Sys.getenv("TESTTHAT"), "true")) {
if (identical(Sys.getenv("TESTTHAT"), "true") && identical(getOption("test_package_name"), getPackageName())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is nice, but would have to be methods::getPackageName() (why is this not flagged?), and that function does too much too.

Essentially, methods::getPackageName() calls environmentName(topenv()), that looks better to me than my initial proposal.

@jeanetteclark

jeanetteclark commented Jul 17, 2024

Copy link
Copy Markdown
Contributor Author

No worries. I'll work that in at some point today.

One thing to note - I removed the hms package and ran the tests locally, but a bunch failed. I'm not sure how far this particular bit of code is supposed to go in terms of skipping tests, but all the failures were from DBItest.

@krlmlr krlmlr changed the title Add option to test environment that can be retrieved to prevent tests from skipping unexpectedly in other packages fix: Ensure that testthat::skip() is only called in RSQLite's own tests, never in the tests for other packages Jul 17, 2024
@krlmlr
krlmlr merged commit 96828e3 into r-dbi:main Jul 18, 2024
@krlmlr

krlmlr commented Jul 18, 2024

Copy link
Copy Markdown
Member

Thanks for pushing through!

I don't understand why removing hms fails the tests because this package is really only used with specific dbConnect() arguments, AFAICT. And there's a successful run on CI/CD without hms: https://github.com/r-dbi/RSQLite/actions/runs/9979145876/job/27577869756 .

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unexpected message and test skipping when using extended_types = TRUE

2 participants