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

Add chromote as headless browser for testing #126

Merged
merged 96 commits into from
Aug 27, 2024
Merged

Conversation

siddhesh195
Copy link
Contributor

Use chromote as a headless browser for running tests and migrate tests to work with chromote.

Closes #124

Closes #125

@siddhesh195 siddhesh195 self-assigned this Jun 6, 2024
@tdhock
Copy link
Collaborator

tdhock commented Jun 7, 2024

this is ok but please

  • use a more informative commit message (describing what you changed) instead of just Update file.R (which only tells us the file you changed). Are you using the github web interface? please instead use git on the command line
  • please push commits and create PR as soon as you make them, instead of waiting for the code to work completely. It is important in open-source to share your partial results, not just the final result.

@siddhesh195
Copy link
Contributor Author

this is ok but please

  • use a more informative commit message (describing what you changed) instead of just Update file.R (which only tells us the file you changed). Are you using the github web interface? please instead use git on the command line
  • please push commits and create PR as soon as you make them, instead of waiting for the code to work completely. It is important in open-source to share your partial results, not just the final result.

I use GitHub web client and it automatically puts the commit message. I will put a meaningful commit message going forward. I will also createa PR with partial updates instead of waiting for the final solution. Thanks for letting me know of this and I was not aware of this process

@tdhock
Copy link
Collaborator

tdhock commented Jun 8, 2024

@Faye-yufan maybe you can schedule a google meet with @siddhesh195 to discuss / demo how to use the git command line instead of the web interface to push / commit etc?

@tdhock
Copy link
Collaborator

tdhock commented Jun 8, 2024

the code looks reasonable but I have not had time to do a detailed review.
@siddhesh195 can you please add docs about how to use the chromote interactively?
what software do I need to install?
how do I start the chromote web browser window?
how do I run a test and see the animint in the web browser window?
Can you please write some docs? maybe on the wiki/Testing page? (with screenshots if possible)

@siddhesh195
Copy link
Contributor Author

the code looks reasonable but I have not had time to do a detailed review. @siddhesh195 can you please add docs about how to use the chromote interactively? what software do I need to install? how do I start the chromote web browser window? how do I run a test and see the animint in the web browser window? Can you please write some docs? maybe on the wiki/Testing page? (with screenshots if possible)

added the documentation for chromote testing. the link where I added it is: https://github.com/animint/animint2/wiki/Testing

@tdhock
Copy link
Collaborator

tdhock commented Aug 15, 2024

I'm getting the following error on compiler test, why?

ENABLED
Serving the directory /home/runner/work/animint2/animint2/tests/testthat at http://127.0.0.1:4848/
Error in `startup()`:
! Chrome debugging port not open after 10 seconds.

also was getting that for renderer1 previously, and also currently getting it from renderer2.

@siddhesh195
Copy link
Contributor Author

I'm getting the following error on compiler test, why?

ENABLED
Serving the directory /home/runner/work/animint2/animint2/tests/testthat at http://127.0.0.1:4848/
Error in `startup()`:
! Chrome debugging port not open after 10 seconds.

also was getting that for renderer1 previously, and also currently getting it from renderer2.

It is some GitHub Actions issue. Never got such error on my local machine but sometimes I get it on GitHub Actions.

@tdhock
Copy link
Collaborator

tdhock commented Aug 19, 2024

if such errors happen on github actions often, then we should find out why, debug and fix these false positives before merging this PR.
currently there are no such false positives on github actions with the phantomjs, so I would keep that until that is fixed. what do you think?

@siddhesh195
Copy link
Contributor Author

if such errors happen on github actions often, then we should find out why, debug and fix these false positives before merging this PR. currently there are no such false positives on github actions with the phantomjs, so I would keep that until that is fixed. what do you think?

I found a discussion about this error on chromote with some suggested workarounds: rstudio/chromote#124
I will spend time investigating this further

@tdhock
Copy link
Collaborator

tdhock commented Aug 19, 2024

rstudio/chromote#124 seems like the same error message but maybe different issue because they do not talk about github actions?

@siddhesh195
Copy link
Contributor Author

rstudio/chromote#124 seems like the same error message but maybe different issue because they do not talk about github actions?

I found another discussion where the error is reproducible for Github actions: rstudio/chromote#114
Let me try the suggested workaround of increasing the timeout

@tdhock
Copy link
Collaborator

tdhock commented Aug 26, 2024

hi again so it looks like the build is working now.
that is not great that it sometimes randomly fails.
did you want to make any other changes before I merge this PR?

@siddhesh195
Copy link
Contributor Author

hi again so it looks like the build is working now. that is not great that it sometimes randomly fails. did you want to make any other changes before I merge this PR?

During my last commit, I increased the initialization timeout for chromote. After the submission, I ran the Github build 4 times. The random failures do not happen now. If the last commit looks good, there are no more changes to this PR

@tdhock
Copy link
Collaborator

tdhock commented Aug 27, 2024

great, thanks, that is good news.
code looks good to me.
please add your name under Authors@R in DESCRIPTION, and then I will merge.

@siddhesh195
Copy link
Contributor Author

siddhesh195 commented Aug 27, 2024

great, thanks, that is good news. code looks good to me. please add your name under Authors@R in DESCRIPTION, and then I will merge.

I already added my name to Authors@R in the DESCRIPTION as a part of an earlier #117

@tdhock tdhock merged commit 419cfac into master Aug 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants