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

Create a CT client based on Puppeteer. #143

Closed
zepumph opened this issue May 18, 2022 · 15 comments
Closed

Create a CT client based on Puppeteer. #143

zepumph opened this issue May 18, 2022 · 15 comments

Comments

@zepumph
Copy link
Member

zepumph commented May 18, 2022

This way, we wouldn't need to have to kill all chrome. Related to phetsims/perennial#268.

@jonathanolson and I also noticed that we could potentially use selenium, and or a headless firefox implementation with the same interface for CT's client side module.

@zepumph
Copy link
Member Author

zepumph commented May 19, 2022

I see that I, for fun, made a local Puppeteer client that is already doing a lot of what we want here:

https://github.com/phetsims/aqua/blob/042160003a1372489991e1e347bbebfe3322a1c9/js/local/puppeteerHelpCT.js

I'll start with that.

zepumph added a commit to phetsims/perennial that referenced this issue Jun 1, 2022
zepumph added a commit that referenced this issue Jun 1, 2022
@zepumph
Copy link
Member Author

zepumph commented Jun 1, 2022

Good progress today! I made ContinuousServerClient which uses Workers to kick off instances of Puppeteer. I was able to test this locally and saw my results showing up on CT's report at https://bayes.colorado.edu/continuous-testing/aqua/html/continuous-report.html?maxColumns=5. From here I would like @jonathanolson to review.

  • My main questions are about all of the options passed to chrome in the current headless testing:

google-chrome --headless --disable-gpu --disable-software-rasterizer --remote-debugging-port=94${1} --enable-precise-memory-info --enable-logging=stderr --v=1 "https://bayes.colorado.edu/continuous-testing/aqua/html/continuous-loop.html?id=Bayes%20Chrome"

Are there any of these that we should transfer in to Puppeteer for our purposes? It would be easy to pass them in via puppeteer.launch(); (https://github.com/puppeteer/puppeteer/blob/v14.1.1/docs/api.md#puppeteerlaunchoptions).

Even before the review is complete, I'm going to start it up on CT with less clients, and delete just a couple of the chrome screens. This way we can start to get some on-server testing to see if this works.

If all goes well with the review, next steps are to convert all headless chromes to this process, and to remove the killall chrome cronjob. This will most likely fix #140.

@zepumph
Copy link
Member Author

zepumph commented Jun 1, 2022

  • Another question: Should we have each puppeteer load last longer than 15 minutes?

zepumph added a commit that referenced this issue Jun 1, 2022
@zepumph
Copy link
Member Author

zepumph commented Jun 1, 2022

We are not running 2 clients on bayes with pm2 called continuous-client. Over to you @jonathanolson.

Here is what the logs look like: Each client restarts every 15 minutes:

7|continuo | 2022-06-01T14:14:29: Running "client-server" task
7|continuo | 2022-06-01T14:14:29: New worker instance: 0
7|continuo | 2022-06-01T14:14:29: New worker instance: 1
7|continuo | 2022-06-01T14:29:30: Worker instance complete: 1
7|continuo | 2022-06-01T14:29:30: Worker instance complete: 0
7|continuo | 2022-06-01T14:30:00: New worker instance: 2
7|continuo | 2022-06-01T14:30:00: New worker instance: 3

zepumph added a commit to phetsims/perennial that referenced this issue Jun 1, 2022
@zepumph
Copy link
Member Author

zepumph commented Jun 1, 2022

  • If we get the go-ahead, I will want to update documentation about this procress instead of using the loop shell script.

@jonathanolson
Copy link
Contributor

  • --enable-precise-memory-info is useful for the memory leak detection (?memoryLimit query parameter for sims)
  • --enable-logging=stderr seems like it was used for console output and could be dropped
  • --disable-gpu --disable-software-rasterizer should probably be dropped if it works (and we could have webgl sims work)
  • --remote-debugging-port was for debugging the processes. Puppeteer integration probably makes this irrelevant.

@zepumph
Copy link
Member Author

zepumph commented Jun 2, 2022

I was looking through CT results and felt confident enough to transfer all testing onto this client. I also felt like there is potential that this switch may help solve the issues we are having over in https://github.com/phetsims/special-ops/issues/228. @jonathanolson please continue with the review knowing that this client is our entire testing client for CT at this time (16 instances of puppeteer).

@zepumph
Copy link
Member Author

zepumph commented Jun 2, 2022

Something I'm noticing in the logs is that there are a fair number of "info: puppeteer error: Error: Page crashed!" We now have logs to see that this is happening (~15 times in 15 minutes). I see two possible reasons:

  1. Puppeteer doesn't like having 16 instances running in parallel (I don't think this is the problem)
  2. This demonstrates that there is a CT test that is breaking the testing harness and may or may not be reporting back before the page fails and has to restart. In this case, we should learn a bit more with perhaps a bit more error logging.
5|continuous-client  | 2022-06-02T11:20:54: info: puppeteer error: Error: Page crashed!
5|continuous-client  | 2022-06-02T11:20:54: Message from puppeteerClient: Error: Error: Page crashed!
5|continuous-client  | 2022-06-02T11:20:54:     at /data/share/phet/continuous-client/perennial/js/common/puppeteerLoad.js:58:14
5|continuous-client  | 2022-06-02T11:20:54:     at /data/share/phet/continuous-client/perennial/node_modules/puppeteer/lib/cjs/vendor/mitt/src/index.js:51:62
5|continuous-client  | 2022-06-02T11:20:54:     at Array.map (<anonymous>)
5|continuous-client  | 2022-06-02T11:20:54:     at Object.emit (/data/share/phet/continuous-client/perennial/node_modules/puppeteer/lib/cjs/vendor/mitt/src/index.js:51:43)
5|continuous-client  | 2022-06-02T11:20:54:     at Page.emit (/data/share/phet/continuous-client/perennial/node_modules/puppeteer/lib/cjs/puppeteer/common/EventEmitter.js:72:22)
5|continuous-client  | 2022-06-02T11:20:54:     at Page._onTargetCrashed (/data/share/phet/continuous-client/perennial/node_modules/puppeteer/lib/cjs/puppeteer/common/Page.js:325:14)
5|continuous-client  | 2022-06-02T11:20:54:     at /data/share/phet/continuous-client/perennial/node_modules/puppeteer/lib/cjs/puppeteer/common/Page.js:155:57
5|continuous-client  | 2022-06-02T11:20:54:     at /data/share/phet/continuous-client/perennial/node_modules/puppeteer/lib/cjs/vendor/mitt/src/index.js:51:62
5|continuous-client  | 2022-06-02T11:20:54:     at Array.map (<anonymous>)
5|continuous-client  | 2022-06-02T11:20:54:     at Object.emit (/data/share/phet/continuous-client/perennial/node_modules/puppeteer/lib/cjs/vendor/mitt/src/index.js:51:43)

@jonathanolson
Copy link
Contributor

Any idea which page is crashing?

@zepumph
Copy link
Member Author

zepumph commented Jun 2, 2022

Note I stopped this because of #145.

@zepumph
Copy link
Member Author

zepumph commented Jun 2, 2022

Restarted, see #145 (comment)

@zepumph
Copy link
Member Author

zepumph commented Jun 6, 2022

All has been good on the server. Over the weekend it looks like there were no issues with the client. We are just ready for a review now.

@jonathanolson
Copy link
Contributor

I'm not sure I agree with phetsims/perennial@34bcd56. Usually things called error are a subtype of Error. Is that the case? (We could just resolve/reject with the error instead of wrapping it).

@jonathanolson
Copy link
Contributor

Looks good, although I don't know how parentPort.postMessage( error ); in the client would activate code in the server to note the error. How would that work? Is that just activating the "message"?

@jonathanolson jonathanolson removed their assignment Jun 7, 2022
zepumph added a commit to phetsims/perennial that referenced this issue Jun 9, 2022
zepumph added a commit that referenced this issue Jun 9, 2022
@zepumph zepumph assigned jonathanolson and unassigned zepumph Jun 9, 2022
@jonathanolson
Copy link
Contributor

Looks like this review was handled with commits above. Closing.

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

No branches or pull requests

2 participants