Tests not firing in the remote #523
Replies: 4 comments 6 replies
-
@jason-capsule42 replied Thanks for summarizing here Dale. Regarding your points at the end:
|
Beta Was this translation helpful? Give feedback.
-
re: release issue The failed release https://github.com/AlaskaAirlines/auro-dropdown/actions/runs/8161376207. With additional research, I discovered that the issue was caused by the following commit. It appears that @irma-kurnia-alaska had removed the lining bypass rules. I have created the issue AlaskaAirlines/auro-dropdown#222 to solve. Please note that this update was done with this commit AlaskaAirlines/auro-dropdown@7039e37 Which was also updated with this commit AlaskaAirlines/auro-dropdown@a7aed9c Which was originally added with this commit AlaskaAirlines/auro-dropdown@13ec34e I am unsure why this listing bypass was added, removed, added back, and then removed again. What concerns me is that a listing bypass for an accessibility warning was added and there is no linked issue or body in the commit message describing why this was bypassed versus addressed. This is requested with issue AlaskaAirlines/auro-dropdown#222 |
Beta Was this translation helpful? Give feedback.
-
re: tests in the remote not firing The problemWith this commit, The The Both of these were removed with the intention of making the WC-Generator/template/.husky/pre-commit.temp
4:./node_modules/.bin/npm-run-all preCommit test linters postinstall The HUGE miss here was that when these were removed, there was no backup plan where linters were running in the remote. This became 100% dependent on local developers to always run the pre-commit. All repos have been updated to support ProposalI am still in favor of running a speedy build when needed. Adding linters and tests will effectively slow things down. Developers would use "build": "npm-run-all build:sass sass:render types", I propose a new script, "build:test": "npm-run-all test linters", I propose a new script, "build:release": "npm-run-all build build:test build:api build:docs build:demoScripts bundler postinstall", or if "build:release": "npm-run-all build build:test build:api build:docs bundler types postinstall", I am suggesting removing Lastly, update "build:ci": "npm-run-all sweep build:release", An updated name: Test and publish
# Controls when the action will run. Triggers the workflow on push or pull request
# events but only for the main branch
on:
push:
branches: [ main ]
pull_request:
branches: [ main ]
# A workflow run is made up of one or more jobs that can run sequentially or in parallel
jobs:
test:
runs-on: ubuntu-latest
strategy:
matrix:
node-version: [18.x, 20.x]
steps:
- uses: actions/checkout@v4
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
- run: npm ci
- run: npm run build
- run: npm run build:test
release:
# Only release on push to main
if: github.event_name == 'push' && github.ref == 'refs/heads/main'
runs-on: ubuntu-latest
needs: test
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-node@v4
with:
node-version: 20
- run: npm ci
- run: npm run build:release
- uses: cycjimmy/semantic-release-action@v4
env:
GITHUB_TOKEN: ${{ secrets.ACCESS_TOKEN }}
NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
|
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
The problem
@jason-capsule42 merged a PR AlaskaAirlines/auro-dropdown#217 and rained questions about why this release failed. https://github.com/AlaskaAirlines/auro-dropdown/actions/runs/8161376207
There are a couple of things here. One is the new a11y linting error. I believe that this was caused by a dependency update. Which one, not sure. Still looking into that.
This is what put a spotlight on the issue. What is weird is that the Node 18 and 20 tests didn't fail, but the release did. Why? Also, why did this not fail locally on Jason's machine when a pre-commit should have caught that?
re: Jason's local, I can only assume that with the recent dependency updates his local was out of sync. That will happen. Also, understand that this new error is not a recent code change, the error is appearing because a linting dependency update now sees it. Yay, updated tools finding years-old errors. This is how we get better.
What this REALLY surfaced is another interesting fact. Why did this ONLY fail on release? This gets a little deep, I will do my best here.
Recently I made some updates to our repos that will post back current released versions into the documentation. In order to do this I need to build the docs in the remote. Pretty straightforward.
To trigger this, there is a new command in the ./package.json called
postversion
. What this does is, in the same action as the semantic-release process, a build and a commit are happening in the remote (Github). This happens here https://github.com/AlaskaAirlines/auro-dropdown/actions/runs/8161376207/job/22310029584#step:6:228Like I said before, the pre-commit that usually happens locally to validate a commit is now also happening in the remote. And if you have the updated dependencies, you will get the same error.
Conclusion: I can't say that this is happening in all repos, but it's happening. Tests and linters MUST run in the remote with the Node 18/20 builds. It's great that we are getting successful builds, but not running tests, that's bad. That must be fixed.
What I feel happened is that test and linters were removed from the build script to keep local builds fast. I get that. But with revisions to the GitHub action workflow over time, we forgot about that change and never maintained those actions.
Recommentation
The following options are on the table.
I welcome your feedback.
Beta Was this translation helpful? Give feedback.
All reactions