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

Make puppeteer work in CI #15

Merged
merged 16 commits into from
Jul 15, 2019
Merged

Make puppeteer work in CI #15

merged 16 commits into from
Jul 15, 2019

Conversation

orlando
Copy link
Contributor

@orlando orlando commented Jul 4, 2019

What: Make puppeteer work in CI

Why: Close #12, Close #23

How:

  • Refactor PDFEngine to use puppeteer
  • Refactor PDFEngine to return Buffer instead of file paths
  • Refactor interfaces
  • Changes to test to go along code changes

@orlando orlando force-pushed the od/ci branch 3 times, most recently from 7183377 to d60970c Compare July 4, 2019 21:38
@orlando orlando changed the title [WIP] Make puppeteer work in CI Make puppeteer work in CI Jul 10, 2019
@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #15 into master will increase coverage by 15.86%.
The diff coverage is 89.13%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #15       +/-   ##
========================================
+ Coverage   64.13%   80%   +15.86%     
========================================
  Files          17    17               
  Lines          92   105       +13     
  Branches        3     4        +1     
========================================
+ Hits           59    84       +25     
+ Misses         30    18       -12     
  Partials        3     3
Impacted Files Coverage Δ
src/documents/UnauthorizedSignForm.js 100% <100%> (ø) ⬆️
src/documents/PrivateStudentLoan.js 100% <100%> (ø) ⬆️
src/engines/PDFEngine.js 100% <100%> (+46.15%) ⬆️
src/documents/CreditReportDispute.js 100% <100%> (ø) ⬆️
src/documents/TaxOffsetReview.js 100% <100%> (ø) ⬆️
src/documents/FalseCertAbilityToBenefit.js 100% <100%> (ø) ⬆️
src/documents/WageGarnishment.js 100% <100%> (ø) ⬆️
src/worker.js 100% <100%> (ø) ⬆️
src/documents/FalseCertDisqualifying.js 100% <100%> (ø) ⬆️
src/documents/GeneralDispute.js 100% <100%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5398d87...b146f28. Read the comment docs.

Copy link
Contributor

@duranmla duranmla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, nevertheless, I am also concern about the addition of puppeteer as a dependency. I am not certain if it actually makes any actual conflict with the serverless deployment for terms by any meaning.

Did you try to deploy?

Probably we skip the fact of adding the whole puppeteer for something although it may not affect at all because I can't find any note related to it in my journal (but I still have a small doubt about it)

- libnss3
sudo: required
node_js:
- "8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we care about the 8.16 vs 8 versions (we have 8.16 defined in our node-version and our service)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should test against the latest version in the 8 branch (currently is 8.16), this way we can know is safe to update when the release one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so we should update this line in the CI to be 8.16? or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I think it's fine, this way we can test new versions of Node while having our build green. If something breaks we will know

@duranmla
Copy link
Contributor

@orlando few comments before approve. Take a look and let me know.

@orlando
Copy link
Contributor Author

orlando commented Jul 15, 2019

@duranmla the puppeteer library is added as a devDependency, so we use that library while developing.

The chrome-aws-lambda one is used in production and is installed under dependencies. This is what they suggest in their repo https://github.com/alixaxel/chrome-aws-lambda/wiki/HOWTO:-Local-Development#workaround

@duranmla duranmla self-requested a review July 15, 2019 19:26
duranmla
duranmla previously approved these changes Jul 15, 2019
Copy link
Contributor

@duranmla duranmla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the minor update, I think we are cool to go.

@orlando orlando merged commit 1c49452 into master Jul 15, 2019
@orlando orlando deleted the od/ci branch July 15, 2019 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the puppeteer instance to run as part of PDFEngine Add support for chrome operations in our CI pipeline
2 participants