Skip to content

Run npm test on linux and macos - #164

Merged
robertbrignull merged 2 commits into
mainfrom
npm_test_os
Aug 28, 2020
Merged

Run npm test on linux and macos#164
robertbrignull merged 2 commits into
mainfrom
npm_test_os

Conversation

@robertbrignull

@robertbrignullrobertbrignull commented Aug 28, 2020

Copy link
Copy Markdown
Contributor

A change I made in #162 accidentally broke the unit tests on non-linux systems.

I think running just this on all development operating systems will be fine. We don't need to do the other jobs from this workflow.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@robertbrignull

robertbrignull commented Aug 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Oh, didn't realise the bug was already on this branch. Should be fixed now. I'm not sure if this is exactly the right fix as it does feel like we're just duplicating the implementation a bit. Alternative would be just deleting this test, but I think let's keep it for now.

@robertbrignull

robertbrignull commented Aug 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Ok. For now I've scaled this back to only running on linux and osx. We should get the tests running on windows too but it seems there are a bunch of failures and it'll be more effort.

aeisenberg
aeisenberg reviewed Aug 28, 2020
View reviewed changes
Comment thread.github/workflows/pr-checks.yml
runs-on: ubuntu-latest
strategy:
matrix:
os: [ubuntu-latest,macos-latest]

@aeisenbergaeisenbergAug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason why windows isn't included here?

@robertbrignullrobertbrignullAug 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The tests don't currently pass on windows and I think it's going to be a bit of effort to get them working. I want to do that but we may as well merge this now and do windows separately later.

@aeisenbergaeisenbergAug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure.

@robertbrignullrobertbrignull changed the title Run npm test on all operating systemsRun npm test on linux and macosAug 28, 2020
@robertbrignull
robertbrignull merged commit 5bd2832 into mainAug 28, 2020
@robertbrignull
robertbrignull deleted the npm_test_os branch August 28, 2020 15:57
@github-actionsgithub-actionsBot mentioned this pull request Sep 1, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeisenbergaeisenbergaeisenberg approved these changes
+1 more reviewer
@chrisgavinchrisgavinchrisgavin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chrisgavinchrisgavin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@robertbrignull@aeisenberg@chrisgavin