-
Notifications
You must be signed in to change notification settings - Fork 19
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
e2e testing + nodejs ci #155
Conversation
Blocked until #145 is merged |
@erikyo hmm, looks like npm install is failing on build, could you take a look? |
@erikyo this is still failing on node 14. However I'm wondering if we should just drop this test altogether tbh. Node 14 goes out of support next month, and it's not like PHP where we should be prepared to run it on different end user environments, particularly with a .nvmrc file. What do you think? |
I think isn't critical, but on the other hand I wanted to stay in line with Gutenberg which is on node 14 yet. Unfortunately I haven't had time to look at this PR last days but I believe that a good solution for the future would be to reorganise the dependencies better, perhaps by moving the storybook stuff into a separate package.json (inside the stories folder). Maybe we could add the build test for different OS that can be more useful compared to an old node version! @johnhooks, you are too of the idea of removing node 14, is that right? |
@erikyo yes I think I told you something almost identical to @Sephsekla points 😆 in a DM. Though I agree if Gutenberg doesn't plan to bump their required Node version, it would be best to figure it out. Is anyone knowledgeable about the status of bumping to 16? |
Looks like it's in the works, it might even get bumped to 18 🎉. I would drop it and we can revisit the decision when/if it ever becomes a problem. |
OK, looking through the WP Core PR I'd agree. A little tempted to upgrade the nvm version to 18 as well. @erikyo if you can update the PR with the latest branch then I can approve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💥
What?
this PR is for adding e2e tests to wp-notify
Why?
As discussed recently on slack, we think it may be convenient to check beforehand the correctness of contributions and, if we decide to activate the auto-update of depend bots, to check that suggested packages do not break the plugin
#154 - Add e2e tests
How?
I added jest and puppeteer and with these I created (thanks to @wordpress/e2e-tests) two 'projects', one for unit tests and the other for e2e tests (see the jest.config.js file for more info). Then I added in package.json the 'test' script that simply launches jest (both projects)
As far as CI is concerned, I moved what was related to js to 'node-ci.yml' and then i added the e2e test job after the build testing