Skip to content

ci: also run the tests against the latest shfmt - #1394

Open
kolyshkin wants to merge 2 commits into
bash-lsp:mainfrom
kolyshkin:ci-shfmt-matrix
Open

ci: also run the tests against the latest shfmt#1394
kolyshkin wants to merge 2 commits into
bash-lsp:mainfrom
kolyshkin:ci-shfmt-matrix

Conversation

@kolyshkin

Copy link
Copy Markdown
Contributor

Builds on #1393 — please merge that first, otherwise the new job is red on arrival. The first commit here is that PR's.

Why

The formatter tests assert on shfmt's output and on its error messages, and both change from time to time. CI installs shfmt from the Ubuntu archive, which lags well behind upstream, so such a change is only noticed once someone on a more current distribution runs the test suite and reports it — which is exactly how #1393 came about (Fedora ships shfmt 3.13.1; the archive is older).

What

One extra job running against the latest shfmt release. It is an include: entry rather than a second matrix axis, so this costs a single job rather than doubling the matrix:

verify (node 20.x, distro shfmt)
verify (node 22.x, distro shfmt)
verify (node 22.x, latest shfmt)

Notes:

  • The archive shfmt is now installed only in the jobs that ask for it, so the two are never on PATH together and the result does not depend on /usr/local/bin preceding /usr/bin.
  • fail-fast: false, so a break in the latest-shfmt job does not cancel the others.
  • The codecov upload is pinned to one job, as before.
  • A step prints command -v shfmt and shfmt --version, so a failure log says up front which binary produced it.

Tradeoff worth a maintainer opinion

Resolving "latest" at run time means a new shfmt release can turn a PR red for reasons unrelated to that PR. That is the point of the job, but if you would rather have deterministic CI, the alternative is pinning a version here and letting renovate bump it — the signal arrives as a renovate PR instead of on someone else's. Happy to switch it over if you prefer that.

@skovhus
skovhus enabled auto-merge August 19, 2026 07:50
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.56%. Comparing base (d14b5d2) to head (a3ed74f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1394   +/-   ##
=======================================
  Coverage   80.56%   80.56%           
=======================================
  Files          29       29           
  Lines        1513     1513           
  Branches      374      374           
=======================================
  Hits         1219     1219           
  Misses        235      235           
  Partials       59       59           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kolyshkin

Copy link
Copy Markdown
Contributor Author

@skovhus alas to merge this job you need to edit repo's branch protection rules (I don't see those but they should be at https://github.com/bash-lsp/bash-language-server/settings/branches), removing the old ones (verify (20.x) and verify (22.x)) and add new ones (they will be listed there, not sure how they are called).

Alternatively, I can add a commit here which adds a simple success (or all-done) job which requires all other jobs to succeed, and so you'll only need to require a single all-done job. For example, see opencontainers/runc#4376

auto-merge was automatically disabled August 19, 2026 08:48

Head branch was pushed to by a user without write access

@kolyshkin

Copy link
Copy Markdown
Contributor Author

Pushed the second commit implementing all-done, let me know if you want it or not @skovhus

The formatter tests assert on shfmt's output and on its error messages,
both of which change from time to time. The shfmt in the Ubuntu archive
lags well behind, so those changes are only noticed once a user on a
more current distribution reports the failure.

Add one job that runs against the latest shfmt release. It is an extra
matrix entry rather than a second axis, so this costs a single job
rather than doubling the matrix.

The archive shfmt is now installed only in the jobs that ask for it, so
that the two never end up on PATH together.
The sole purpose is to make branch protection rules simple: instead of
requiring jobs which names can change (like in the previous commit), add
a summarizing job, which is the only one that should be required.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants