-
Notifications
You must be signed in to change notification settings - Fork 20
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
stringsAsFactors=TRUE #34
Conversation
also need to fix:
|
I've added a commit with the working driver functions. Such errors needed to be fixed: https://gist.github.com/lazycipher/2e3d48b16de3a44969ebf84e7481a212#file-output-L32 |
hi for this one you need to figure out where the Rmd file is being generated, try to run the code line by line yourself, where does the Rmd show up?
|
for most of the other ones looks like you are missing some data sets.
these files are all in https://github.com/tdhock/animint2/tree/master/data but they are not included in the "built" version of the package which is created by this script https://github.com/tdhock/animint2/blob/master/build.sh because we need to reduce pkg size for upload to CRAN. However these tests should run fine on travis / your machine (don't delete these data sets) |
on CRAN we only run the "compiler" test files https://github.com/tdhock/animint2/blob/master/build.sh#L24 |
Rmd.file <- system.file("examples", "test_knit_print.Rmd", package = "animint2")
index.file <- file.path("animint-htmltest", "index.Rmd")
file.copy(Rmd.file, index.file, overwrite=TRUE) After running these commands the value of Rmd.file is empty
The |
first of all please use system.file(mustWork=TRUE) so you get an error (instead of an empty string) when the file does not exist. second of all those data files do exist in the github repo, so they must be getting lost somewhere in the package installation. How is the package being installed? If build.sh is being used then that is a problem because it deletes some data files which are necessary for testing on Travis (but which are too large for CRAN). Please use R CMD INSTALL path/to/animint2-directory |
Okay got it! I was installing using the archive tar file. But now installed using the repo code as you told me. |
ok there still seem to be several failures, can you please investigate for each failure please determine whether it is
At some point in time all of these tests were passing, so I suspect that most of these are true test failures, and there are some minor changes to the rendering code that we must do. It is helpful to use a browser with developer tools for the headless browser testing, so you can inspect the rendered html and see what it looks like, have you ever done that? https://developer.mozilla.org/en-US/docs/Tools |
Yes I've used developer tools. |
@tdhock, I'm getting this error when using geom_point() from animint2. scatter <- ggplot()+
+ geom_point(
+ mapping=aes(x=life.expectancy, y=fertility.rate, color=region),
+ data=WorldBank1975)
> scatter
Following this simple example: Example |
yes this is a known issue with R-4 #31 so please investigate and fix. ggplot2 fixed these grid unit related issues already so you should check their code e.g. https://github.com/tidyverse/ggplot2/blob/master/R/margins.R to see how they fixed it. |
Thanks for letting me know. Let me look around to solve this issue. |
Making changes in margin worked. I was working on the issue related to
I found out that the code is written here L16 returns the first entry of array which should return the whole array with all values so that we can calculate the difference. Updating the function and removing [[1]] makes it work.
while Making the change passes the test:
|
In the The code is being executed and the animated vizs are opening in other tabs. |
great work about the margin fix! |
I noticed that many errors are because of remDr$executeScript(sprintf("document.getElementById('%s').click()", as.character(id))) where |
hi where are your commits? I don't see any commits since May. Can you please your fixes to this branch? |
I haven't committed and pushed! let me do that asap! |
@@ -71,6 +71,7 @@ S3method(heightDetails,titleGrob) | |||
S3method(heightDetails,zeroGrob) | |||
S3method(interleave,default) | |||
S3method(interleave,unit) | |||
S3method(knit_print,animint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @lazycipher this commit fixes the Rmd rendering problems for me. does it work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! It's fixed now!
glad to see tests are passing on travis of your fork,but what is the issue/difference?
does your fork do that/work on that line? |
Yes it's working on forked one. The only difference I can see is the url that is being called. It includes the |
hi maybe you should remove this line in the DESCRIPTION which tells devtools to install a specific (maybe incompatible?) version of RSelenium
|
DESCRIPTION
Outdated
rpart, | ||
svglite | ||
Enhances: sp | ||
Remotes: ropensci/RSelenium@v1.7.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdhock, I've already changed this version thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in your fork do you have @v1.7.4? i'm confused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and I pushed the same in r4-fixes branch in this repo as well.
https://travis-ci.org/github/lazycipher/animint2/jobs/701227388#L1067
@tdhock, Can you check if there's any config you changed in your Travis dashboard, please? |
not sure what may have changed, but can't you check because you are a collaborator? https://stackoverflow.com/questions/25857012/is-it-possible-to-set-up-travis-for-a-project-for-which-you-are-a-collaborator |
GITHUB_PAT is for the gist test (allows travis to post a gist) |
@tdhock, Okay! Got you! |
I don't understand either. |
@tdhock sounds good! |
once you do get your fork working please do try to port the changes back here (maybe in a new branch/pr?) |
sounds good to me! if you say, I can push commits to this branch only. |
Fixing test issues and R4 fixes
@tdhock, Looks like the |
@faizan-khan-iit, Should we merge this branch to master as all tests are passing? |
great thanks for the update please merge |
Thank you so much! |
fixes