Fixed triple-reporting lint issues

[yzwdroid]

Issue This PR Addresses

Fixes #1336

Type of Change

• Bugfix: Change which fixes an issue
• New Feature: Change which adds functionality
• Documentation Update: Change which improves documentation
• UI: Change which improves UI

Description

only run test-ci when the os is ubuntu

Checklist

• Quality: This PR builds and passes our npm test and works locally
• Tests: This PR includes thorough tests or an explanation of why it does not
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
• Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

[humphd]

The problem I see with this approach is that we no longer run tests on the other platforms, in which case there is no point having them listed in the matrix.

@raygervais do you have any insight into other ways to reduce the lint spam without shutting off the build matrix?

[humphd]

@yzwdroid this needs a rebase, and an update on where we stand. Are you hoping to get this done in time for 0.3? If so, we need to finalize and merge this today.

[yzwdroid]

I am thinking if we have a better solution for this. I already have a PR merged for release 0.3. Thanks.
I'll do the rebase now.

[humphd]

Alright, I did some research and here's a better way, let's separate the lint and test scripts, and do something like Svelte is doing. Here is their workflow file:

name: CI
on: [push, pull_request]
jobs:
  Tests:
    runs-on: ${{ matrix.os }}
    timeout-minutes: 10
    strategy:
      matrix:
        node-version: [8, 10, 12, 14]
        os: [ubuntu-latest, windows-latest, macOS-latest]
    steps:
    - uses: actions/checkout@v1
    - uses: actions/setup-node@v1
      with:
        node-version: ${{ matrix.node-version }}
    - run: npm install
    - run: npm test
      env:
        CI: true
  Lint:
    runs-on: ubuntu-latest
    timeout-minutes: 2
    steps:
    - uses: actions/checkout@v1
    - uses: actions/setup-node@v1
    - run: 'npm i && npm run lint'
  Unit:
    runs-on: ${{ matrix.os }}
    timeout-minutes: 5
    strategy:
      matrix:
        os: [ubuntu-latest, windows-latest, macOS-latest]
    steps:
    - uses: actions/checkout@v1
    - uses: actions/setup-node@v1
    - run: 'npm i && npm run test:unit'

To do this, we'll need to get rid of test-ci and do these three things separately:

1. npm run prettier-check to make sure the PR has proper formatting. Do this only on linux
2. npm run eslint to make sure the PR passes lint. Do this only on linux
3. npm run test to make sure the PR passes tests. Do this on all OSes

[yzwdroid]

Thanks for the help @humphd . I changed the condition as you mentioned.

[humphd]

You haven't done what I asked. Can you split up the jobs so that we have separate prettier, eslint, and test phases that can run in parallel?

[yzwdroid]

I am a little confused about this approach. If we run all the jobs async, then we need to do npm install three times in the ubuntu server. Is that slow down a little bit?

[humphd]

Not if we also add caching, see https://docs.github.com/en/free-pro-team@latest/actions/guides/caching-dependencies-to-speed-up-workflows.

[yzwdroid]

I'll add the cache, and commit it later.

[humphd]

Let's get rid of this build step too, since we aren't using the build in CI.

[humphd]

See b615e5c, where I'm doing that for another PR too.

[yzwdroid]

After I add the cache, the build time reduced from 3m 30 s to 2m 54s on Ubuntu from my repo https://github.com/yzwdroid/telescope/runs/1449300001?check_suite_focus=true. I'm not sure if I have done it right.

[humphd]

Let's get this rebased for the conflict that's in node-js-ci.yml.

[yzwdroid]

I rebased the current commit, Is there any code to be changed?