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

run all node.js tests on GHA #4459

Merged
merged 4 commits into from
Oct 7, 2020
Merged

Conversation

boneskull
Copy link
Contributor

In #4402, @outsideris moved the build off of AppVeyor and onto GHA.

This PR moves Linux Node.js builds off of Travis-CI and on to GHA. They are run in the same matrix as the existing Windows builds. Included are the lint checks and coverage reporting to Coveralls.

I've consolidated a few of the existing jobs to reduce waiting / increase parallelization.

Remaining on Travis-CI are the browser tests, as I'm currently struggling to get SauceLabs tests running in GHA's env.

@boneskull boneskull added the qa label Oct 2, 2020
@boneskull boneskull requested a review from outsideris October 2, 2020 21:15
@boneskull boneskull self-assigned this Oct 2, 2020
@coveralls
Copy link

coveralls commented Oct 2, 2020

Coverage Status

Coverage remained the same at 93.959% when pulling 2a0e1f5 on boneskull/experimental/gh-action into 8f5d6a9 on master.

@boneskull
Copy link
Contributor Author

See #4460 for browser tests on GHA

Copy link
Contributor

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

LGTM except Node.js 10 and 12 disabled.

.github/workflows/mocha.yml Outdated Show resolved Hide resolved
path: '${{ steps.npm-cache.outputs.dir }}'
key: "${{ matrix.os }}-node-v${{ matrix.node }}-${{ hashFiles('**/package-lock.json') }}"
restore-keys: |
${{ matrix.os }}-node-v${{ matrix.node }}-
Copy link
Contributor

Choose a reason for hiding this comment

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

When I test cache on windows before, it took about 80s to download over 600MB cache files.

In this GHA, "Install Dependencies" took like below without cache. So, I worried about using cache takes more time.

Ubuntu

Run npm ci --ignore-scripts
added 2827 packages in 24.523s

Windows

Run npm ci --ignore-scripts
added 2827 packages in 65.224s

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'll profile this and see if we should be caching or no

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turns out windows has a cache of like 600MiB, and linux is 50MiB, so that's why.

I changed this to use the cache for linux but not windows.

@boneskull boneskull force-pushed the boneskull/experimental/gh-action branch 2 times, most recently from 24e1a18 to d7642b8 Compare October 5, 2020 21:02
@outsideris
Copy link
Contributor

Interesting.
Restore cache took under 5s with 53MB on Ubuntu, but it took over 1m with 654MB on windows.

${{ runner.os }}-growl-installer
- name: Add Growl Installer to Path (Windows)
if: "${{ matrix.os == 'windows-2019' }}"
run: echo "C:\Program Files (x86)\Growl for Windows" >> $GITHUB_PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

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 was unable to get this working. Going to try this suggestion now

@boneskull boneskull force-pushed the boneskull/experimental/gh-action branch from 6129400 to 43487ce Compare October 6, 2020 17:54
@github-actions

This comment has been minimized.

@boneskull boneskull force-pushed the boneskull/experimental/gh-action branch 4 times, most recently from 09aa40f to d0202a6 Compare October 7, 2020 20:25
- disables `node_modules` cache on windows cuz its slow
- adds annotation support for Mocha tests (via https://npm.im/mocha-github-actions-reporter) and ESLint checks
- `::add-path::` is deprecated, but the suggested migration does not work; see [ongoing discussion](https://github.jparrowsec.cnmunity/t/migration-from-deprecated-add-path-on-windows/136265)

Signed-off-by: Christopher Hiller <[email protected]>
@boneskull boneskull force-pushed the boneskull/experimental/gh-action branch from 27d46b1 to dac8d50 Compare October 7, 2020 21:34
@boneskull
Copy link
Contributor Author

Still unable to get the new "environment file" working on Windows, so merging as-is.

This PR adds inline annotations for Mocha tests and ESLint checks. This means that you'll see test and lint failures pinned to the relevant line numbers inline, in the diff on GitHub.

Of note, the Mocha annotations arrive via https://github.com/daniellockyer/mocha-github-actions-reporter. This reporter is not added to our devDependencies, and is only installed on GHA as a separate step

@boneskull boneskull merged commit df8e9e6 into master Oct 7, 2020
@boneskull boneskull deleted the boneskull/experimental/gh-action branch October 7, 2020 22:52
@boneskull boneskull added this to the next milestone Oct 7, 2020
@boneskull boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Oct 7, 2020
@boneskull boneskull modified the milestones: next, v8.2.0 Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants