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

Adding github action tests #769

Merged
merged 11 commits into from
Jul 24, 2020

Conversation

kiwiupover
Copy link
Member

@kiwiupover kiwiupover commented Jul 17, 2020

Adding github action tests

  • mocha tests against node 12, 10
  • ember tests
  • windows mocha test against node 12, 10
  • window ember 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

- name: Install NPM version 4
run: |
npm config set spin false
npm install -g npm@4
Copy link
Member

Choose a reason for hiding this comment

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

😭

Comment on lines 64 to 65
- name: NPM Install
run: npm install
Copy link
Member

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?

Comment on lines 44 to 45
- name: NPM Install
run: npm install
Copy link
Member

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:

- yarn --ignore-engines

- name: Run Tests
run: npm run test:ember

test-windows-mocha-node:
Copy link
Member

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...

Comment on lines 25 to 28
test-mocha:
name: Mocha Tests
runs-on: ubuntu-latest

strategy:
matrix:
node-version: [12.x, 10.x]
Copy link
Member

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:

Suggested change
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]

Comment on lines 88 to 89
- name: Remove Yarn Lock File
run: rm yarn.lock
Copy link
Member

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.

- name: Run Mocha Tests
run: npm run test:mocha

test-windows-ember:
Copy link
Member

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.

Comment on lines 21 to 22
env:
CI: true
Copy link
Member

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

@kiwiupover
Copy link
Member Author

@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.

@rwjblue rwjblue merged commit 1d5bb8f into ember-fastboot:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants