-
Notifications
You must be signed in to change notification settings - Fork 161
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
Adding github action tests #769
Adding github action tests #769
Conversation
- name: Install NPM version 4 | ||
run: | | ||
npm config set spin false | ||
npm install -g npm@4 |
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.
😭
.github/workflows/ci.yml
Outdated
- name: NPM Install | ||
run: npm install |
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.
I find it vaguely odd to use npm
here. Can you swap to yarn
?
.github/workflows/ci.yml
Outdated
- name: NPM Install | ||
run: npm install |
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.
Should be running yarn
not npm install
.
See config in .travis.yml:
ember-cli-fastboot/.travis.yml
Line 34 in 19f0c67
- yarn --ignore-engines |
.github/workflows/ci.yml
Outdated
- name: Run Tests | ||
run: npm run test:ember | ||
|
||
test-windows-mocha-node: |
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.
Since nothing is special/different about these mocha tests from the test-mocha
job, we should consolidate them to a single job configuration above. I made an inline suggestion for that...
.github/workflows/ci.yml
Outdated
test-mocha: | ||
name: Mocha Tests | ||
runs-on: ubuntu-latest | ||
|
||
strategy: | ||
matrix: | ||
node-version: [12.x, 10.x] |
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.
We can avoid duplicate job definitions by using a matrix for OS also:
test-mocha: | |
name: Mocha Tests | |
runs-on: ubuntu-latest | |
strategy: | |
matrix: | |
node-version: [12.x, 10.x] | |
test-mocha: | |
name: "Mocha - ${{ matrix.node-version }} - ${{ matrix.os }}" | |
runs-on: "${{ matrix.os }}-latest" | |
strategy: | |
matrix: | |
node-version: [12.x, 10.x] | |
os: [ubuntu, windows] |
.github/workflows/ci.yml
Outdated
- name: Remove Yarn Lock File | ||
run: rm yarn.lock |
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.
Shouldn't need to do this.
.github/workflows/ci.yml
Outdated
- name: Run Mocha Tests | ||
run: npm run test:mocha | ||
|
||
test-windows-ember: |
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.
Should be able to consolidate these in the job above with a matrix.os.
52cf10d
to
4476b5b
Compare
.github/workflows/ci.yml
Outdated
env: | ||
CI: true |
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.
FWIW, I think this is automatically set now
77c2786
to
3441df7
Compare
@rwjblue I have kept mocha tests split between Ubuntu and Windows because I can't get the Windows tests to pass on Github actions. I have combined the ember test into a matrix of os and node versions. |
fc4bb5e
to
185b8d6
Compare
Adding github action tests
This is my github brach running these test. https://github.com/kiwiupover/ember-cli-fastboot/runs/883210806
The mocha window tests are having a hard time.
@rwjblue