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.

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

70 Investigate full coverage for tcplDefine. Updated tcplQuery/tcplDefine for example data #73

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

cthunes
Copy link
Contributor

@cthunes cthunes commented Jun 21, 2023

Removed the check for driver type within tcplDefine and moved it to tcplQuery. tcplQuery accepts a SQL select statement from tcplDefine and can translate it for use with example .rda data using sqldf.

tcplDefine tests are passing and now provide 100% coverage of the function. Closes #70.

@cthunes
Copy link
Contributor Author

cthunes commented Jun 21, 2023

Github actions check failing one test (passing locally) from test-tcplDefine, which is unrelated to the function properly functioning, and a bug with expect_snapshot. This inconsistency will be resolved in a different PR

@cthunes
Copy link
Contributor Author

cthunes commented Jun 21, 2023

See PR #75 for resolution to failing test

…nnotations-retrieval

#24 Data retrieval vignette update
@brown-jason
Copy link
Collaborator

Once we merge the other branches that fix the checks we can rerun the github actions and confirm we are seeing 100% test coverage here.

@cthunes
Copy link
Contributor Author

cthunes commented Jun 27, 2023

Checks are passing with 100% coverage. They are also all passing locally. However, the warning that was appearing here in the Github check as a fail before is now appearing here as WARN result. Looks like this has nothing to do with the snapshot testing anymore, but the warning is probably something we still want to address? Tracing back to sqldf. Perhaps it's 'stringsAsFactors = F' part? Maybe that should be set to true? What do you think @brown-jason?

══ Warnings ════════════════════════════════════════════════════════════════════
── Warning ('test-tcplDefine.R:3:3'): empty returns full dictionary ────────────
no DISPLAY variable so Tk is not available
Backtrace:
     ▆
  1. ├─testthat::expect_snapshot_value(tcplDefine(), style = "serialize") at test-tcplDefine.R:3:2
  2. │ ├─testthat:::with_is_snapshotting(force(x))
  3. │ └─base::force(x)
  4. └─tcpl::tcplDefine()
  5.   └─tcpl::tcplQuery(query = qstring, db = getOption("TCPL_DB"), tbl = tbl)
  6.     ├─data.table::as.data.table(sqldf(query, stringsAsFactors = F))
  7.     └─sqldf::sqldf(query, stringsAsFactors = F)
  8.       └─base::requireNamespace("tcltk", quietly = TRUE)
  9.         ├─base::tryCatch(loadNamespace(package, ...), error = function(e) e)
 10.         │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
 11.         │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 12.         │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
 13.         └─base::loadNamespace(package, ...)
 14.           └─base (local) runHook(".onLoad", env, package.lib, package)
 15.             ├─base::tryCatch(fun(libname, pkgname), error = identity)
 16.             │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
 17.             │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 18.             │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
 19.             └─tcltk (local) fun(libname, pkgname)

@brown-jason
Copy link
Collaborator

Would something like this fix the issue?
forestgeo/learn#186

@cthunes
Copy link
Contributor Author

cthunes commented Jul 7, 2023

Would something like this fix the issue? forestgeo/learn#186

Investigation has been mostly unsuccessful. This link is for users of travis CI, rather than github actions. The closest I could get to travis'

before_install:
- export DISPLAY=:99.0

is github actions':

env:
  DISPLAY: 99.0

No longer getting the same warning that there is no display variable, but instead getting a warning that it "couldn't connect to display "99". I tried 0.0 as well with the same result. Any other ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate how we can get full test coverage for tcplDefine
2 participants