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

Add initial unit and integration tests for CLI and mochify #241

Merged
merged 17 commits into from
Sep 4, 2021

Conversation

mantoni
Copy link
Owner

@mantoni mantoni commented Aug 1, 2021

This adds a few unit tests for the mochify module and some integration tests for the mochify and cli modules and runs them with GitHub actions.

Ref #229

@mantoni mantoni requested a review from m90 August 1, 2021 15:16
@mantoni mantoni marked this pull request as ready for review August 1, 2021 15:16
- name: Install
run: npm ci
- name: Lint
if: matrix.node-version == '16.x'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just for saving some CPU cycles (running it once will be enough) or is there some other reason to it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep. Build speed as well. An alternative could be running lint and prettier in a separate job, but then it needs a to run npm install again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this could turn into a red herring for people that don't know about this and have a build that fails on Node 16, but passes on Node 14, and who then go spelunking in changelogs and everything instead of having a look at the build output.

Probably ok to do it like this still.

.github/workflows/test.yml Show resolved Hide resolved
mochify/test/files.integration.js Show resolved Hide resolved
'--driver',
'playwright',
'--driver-option.engine',
'chromium',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did Firefox do strange things in CI or is this just for speed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

There is no Firefox in the GitHub action container installed. However, chromium is. We'd need to figure out how to install Firefox, but then I thought it's not important for the purpose of this test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was under the impression that Playwright installs all supported engines on postinstall so I am surprised this does not work. It does however place the executable outside of node_modules (as opposed to puppeteer), so maybe this needs some npm caching and/or $PATH adjustments in our context. I can look at that myself if you don't feel like doing it here.

See:

Copy link
Owner Author

Choose a reason for hiding this comment

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

I might have changed too many things at once when I tried to get the build working.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I gave this another try and integration tests time out when I configure firefox as the engine. I'll merge this now and we can look into it separately. Once we have a solution locally and for GitHub Actions, we can document the setup in driver-playwright/README.md.

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.

2 participants