elliotblackburn / mdpdf

Markdown to PDF command line app with support for stylesheets
https://npmjs.com/package/mdpdf
Apache License 2.0
717 stars 47 forks source link

use concurrent file reading and flatten structure #120

Closed nopeless closed 2 years ago

elliotblackburn commented 2 years ago

Hey @nopeless thanks for these they look really good! I've given it a test locally (rebased with the latest master) and I'm seeing some instability in the tests. Every 3rd to 5th run is producing a Failed to launch the browser process! error from Puppeteer. I'm not sure why this is happening and I can't really see how these changes could be causing it but I'm unable to reproduce the issue on master at all.

I'm wondering if we're not awaiting something to be resolved by accident but can't pick it out at first glance. I'll try and dig when I get some time but if you have the capacity to take a look that would be really appreciated!

nopeless commented 2 years ago

Hey @nopeless thanks for these they look really good! I've given it a test locally (rebased with the latest master) and I'm seeing some instability in the tests. Every 3rd to 5th run is producing a Failed to launch the browser process! error from Puppeteer. I'm not sure why this is happening and I can't really see how these changes could be causing it but I'm unable to reproduce the issue on master at all.

I'm wondering if we're not awaiting something to be resolved by accident but can't pick it out at first glance. I'll try and dig when I get some time but if you have the capacity to take a look that would be really appreciated!

can you tell me details about this error?

elliotblackburn commented 2 years ago

Sure, sorry that was silly of me!


Error: Failed to launch the browser process!

TROUBLESHOOTING: https://github.com/puppeteer/puppeteer/blob/master/docs/troubleshooting.md

    at onClose (/Users/elliotblackburn/code/mdpdf/node_modules/puppeteer/lib/Launcher.js:750:14)
    at Interface.<anonymous> (/Users/elliotblackburn/code/mdpdf/node_modules/puppeteer/lib/Launcher.js:739:50)
    at Interface.emit (node:events:402:35)
    at Interface.close (node:readline:586:8)
    at Socket.onend (node:readline:277:10)
    at Socket.emit (node:events:402:35)
    at endReadableNT (node:internal/streams/readable:1343:12)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)```

This was being spat out, from what I can see it looks like it might be only happening on the test `Convert CLI When custom style is passed HTML file contains the default styles when --gh-style is passed` although I'm trying to dig into it a bit deeper and run a few more iterations to be sure.
nopeless commented 2 years ago

image

I manually dug through puppeteer's source code and later realized that the code I have written has nothing to do with the browser. Can you run a test on the commit before this and see if all tests pass on that? (can you test on the code before this change)

elliotblackburn commented 2 years ago

So I found this error on the first run of this PR and subsequent runs didn't turn it up. I then ran the tests on this branch locally 10 times and the error came up roughly every 3rd to 5th run. I did 10 runs on the master branch and didn't get the error at all.

I don't really know how this code could have impacted it and I'll do another check with the latest puppeteer (both on master and this branch) to check but it was coming up consistently within multiple runs which has got me a bit confused.

nopeless commented 2 years ago

@BlueHatbRit https://github.com/nopeless/mdpdf/runs/7043132950?check_suite_focus=true This workflow runs perfectly fine here. Can you show me a failed instance?

elliotblackburn commented 2 years ago

Hm I've taken another crack and can't seem to recreate the issue again, no idea why. I've run the tests 30 times and no issues so I'll go ahead and merge.