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.

Sign up for GitHub

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

Jump to bottom

Replacing Zombie JS with Puppeteer, remove mocha in favour of jest #1652

Merged
merged 1 commit into from
Jul 12, 2021
Merged

Replacing Zombie JS with Puppeteer, remove mocha in favour of jest #1652

merged 1 commit into from
Jul 12, 2021

Conversation

tommrr
Copy link
Contributor

@tommrr tommrr commented Jul 8, 2021

What

  • Replace Zombie.js with Puppeteer
  • Replace Mocha with Jest to be consistence with the rest of the test suite.
  • Update readme, to explain how to run the acceptance tests locally against the dev environments.

How to review

To run the tests follow the section "Executing the acceptance tests against dev environment:" in the new read me.

🚨⚠️ Please do not merge this pull request via the GitHub UI ⚠️🚨

@tommrr tommrr requested review from kr8n3r and paroxp July 8, 2021 15:46
@tommrr tommrr force-pushed the 177790283-Replace-zombie-js-2 branch 5 times, most recently from e1cbc0a to 09ef3ff Compare July 9, 2021 15:39
AP-Hunt
AP-Hunt approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@AP-Hunt AP-Hunt left a comment

Choose a reason for hiding this comment

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

Paired with Tom, got the problematic tests passing. The change itself is good.

kr8n3r
kr8n3r reviewed Jul 12, 2021
View reviewed changes
Copy link
Contributor

@kr8n3r kr8n3r left a comment •
edited
Loading

Choose a reason for hiding this comment

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

while I have some comments, we've had this work going on for a while and it might go on longer so let's just get it done and look to addressing things as part of the PaaS squad

README.md
@@ -71,6 +71,21 @@ Execute the unit tests to ensure everything looks good:
npm test
```

Executing the acceptance tests against dev environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for this addition

acceptance-test-jest-config.json Outdated
@@ -0,0 +1,46 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you can have much smaller config, as it only needs a config for one type of test with a specific setup.
still, it seems to work

Copy link
Member

Choose a reason for hiding this comment

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

Addressing

package.json Outdated
@@ -55,6 +56,7 @@
"@types/passport": "^1.0.7",
"@types/passport-oauth2": "^1.4.11",
"@types/pino": "^6.3.9",
"@types/puppeteer": "^5.4.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

since v7 Puppeteer ship their own types https://github.com/puppeteer/puppeteer#usage-with-typescript

Copy link
Member

Choose a reason for hiding this comment

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

Addressing

package.json
"nock": "^13.1.1",
"nodemon-webpack-plugin": "^4.5.2",
"pegjs": "^0.10.0",
"pegjs-loader": "^0.5.6",
"pino-pretty": "^5.1.1",
"puppeteer": "^9.1.1",
"request-promise-native": "^1.0.9",
Copy link
Contributor

@kr8n3r kr8n3r Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this is a deprecated package, we're hoping to remove. I assume it was added to resolve failing app.test

removing that line would require a rewrite of those tests (remove use of Request)
related: jsdom/jsdom#3092

Copy link
Member

@paroxp paroxp Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the dependency. The refectory of app.test should be addressed outside of this story.

0atman
0atman approved these changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@0atman 0atman left a comment

Choose a reason for hiding this comment

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

Looks great, agreed that @kr8n3r 's changes could be chalked up to debt and handled in a subsequent PR. I'll capture those as issues 👌

paroxp
paroxp reviewed Jul 12, 2021
View reviewed changes
.eslintrc Outdated
@@ -49,7 +49,7 @@
"@typescript-eslint/ban-ts-comment": "warn",
"@typescript-eslint/explicit-function-return-type": "warn",
"@typescript-eslint/naming-convention": ["warn", {"selector": "interface", "format": ["PascalCase"], "custom": {"regex": "^I[A-Z]", "match": true}}],
"@typescript-eslint/no-floating-promises": "error",
"@typescript-eslint/no-floating-promises": "off",
Copy link
Member

Choose a reason for hiding this comment

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

I absolutely disagree with this!

This is here for a reason, as in the past, people have been experiencing a "blank page" whilst using pazmin, due to promises left hanging...

Copy link
Member

Choose a reason for hiding this comment

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

Addressing

Copy link
Contributor

Choose a reason for hiding this comment

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

you're doing gods work 👌

@paroxp paroxp force-pushed the 177790283-Replace-zombie-js-2 branch 3 times, most recently from 3734187 to 0b143a0 Compare July 12, 2021 12:09
@paroxp paroxp force-pushed the 177790283-Replace-zombie-js-2 branch 2 times, most recently from 5ce56e5 to 4c9023f Compare July 12, 2021 12:15
@paroxp paroxp merged commit f816818 into main Jul 12, 2021
@paroxp paroxp deleted the 177790283-Replace-zombie-js-2 branch July 12, 2021 12:26
@kr8n3r kr8n3r mentioned this pull request Jul 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0atman 0atman 0atman approved these changes

@paroxp paroxp paroxp left review comments

@kr8n3r kr8n3r kr8n3r left review comments

@AP-Hunt AP-Hunt AP-Hunt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tommrr @0atman @AP-Hunt @paroxp @kr8n3r