chore: Rename 'ci' script conflicting with NPM

[nschonni]

npm ci without the run is a first party command, so renaming it to prevent confusion

[DavidAnson]

You're right about this, but as probably the only person who types it, I'd prefer to keep it short. I need to think more about this.

[DavidAnson]

npm ci should have been called npm install-ci. The npm team agrees because they named the combination of that and test npm install-ci-test. https://docs.npmjs.com/cli/v6/commands/npm-install-ci-test

[DavidAnson]

Is there a standard name other projects use for this?

[nschonni]

I've seen separate "coverage" jobs/configs in other projects, but I don't think that's all that the extra testing is covering.
I've also seen lint (that fixes) vs. lint-ci (that checks and fails for issues), but nothing really universal

[DavidAnson]

Test and lint are different things. CI is both. Maybe more. I haven't found a name I like better yet.

[nschonni]

Yeah, it was more about seeing ci as a prefix or postfix to script names. Alternately, they just aren't a separate and just are a series of calls in the actual CI like:

    - name: Run All Validations
      if: ${{ matrix.node-version == '14.x' }}
      run: |
        npm run test-cover
        npm run lint
        npm run test-declaration

Although if you were going to go that way, I'd probably have a separate coverage and lint jobs, outside the bigger test matrix

[nschonni]

Did a pass at the separate jobs in a separat commit here, but I can rebase it out.
I usually prefer splitting out lint/coverage jobs, because then they can use the paths filters to only run if *.js, package.json, etc... are changed, rather than each time

[DavidAnson]

I like having a single command for CI so that I can run it locally before commit and know I probably won't break CI. The refactoring of jobs here seems to maybe lose the coverage run? I'd definitely prefer that run everywhere as part of Node/OS platform validation. (Though bugs in c8 limit where it can be run.) You are right that running lint everywhere is unnecessary, but it's quick and it's easier in my mind to define "here is the CI script and it runs on the whole matrix".

[nschonni]

The refactoring of jobs here seems to maybe lose the coverage run?

Right, added that back as an additional job. If you end up using something like Coverall or CodeCov, they have their own GitHub Actions that can be used

"here is the CI script and it runs on the whole matrix"

I think it's actually reversed right now. It does run across all 3 of the OS's, but only for Node 14. It just calls test on the rest right now

[DavidAnson]

If you go back in history, coverage started out running on all matrix combinations. It was only limited like it is now because bugs in c8 on older Node versions came up. The fact it doesn't run on Node 15 is a bug because the version compare isn't (can't be?) done on version ranges. I prefer the one-CI-script approach over separate jobs because it's simple and easy and covers everything possible.

[nschonni]

Just switched to coverage back to all versions to see the error. Is this a bug with c8, or is it just flagging that other versions of node take different paths for that new loading? Alternately, it looks like the block it's complaining about could be ignored with an inline directive https://www.npmjs.com/package/c8#ignoring-the-next-n-elements

[DavidAnson]

c8 bugs, here's an example: bcoe/c8#220

As a bug, the location seems somewhat random and I don't want to put meaningless suppression's in the code every time this comes up.

[DavidAnson]

Sorry, I just can't get away from feeling like "test-ci" is a less accurate name because it suggests this script is a subset of the "test" operations when it isn't because it also does "lint" (and may do other non-test things in the future like code analysis, etc.).

[DavidAnson]

I appreciate your persistence here, but I'm still in a place where I prefer the original approach over the proposed changes. Sorry!

[nschonni]

OK, it's your project