next-build tests#23352
Conversation
|
Checking composer.lock changes... |
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
from
9a8fc8e to
1c94a41
Compare
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
from
ca05a23 to
e8b6fb1
Compare
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
from
e8b6fb1 to
36d9d43
Compare
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
2 times, most recently
from
5bc538c to
ce97073
Compare
|
Checking composer.lock changes... |
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
from
ce97073 to
c3193fa
Compare
|
Checking composer.lock changes... |
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
from
c3193fa to
f3aefad
Compare
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
from
e6e1486 to
a9eb856
Compare
|
Checking composer.lock changes... |
zachbutton
force-pushed
the
zbutton/next-build-tests
branch
from
a9eb856 to
fcc0cbd
Compare
|
Checking composer.lock changes... |
GitHub Workflows (.github/workflows/*.yml)Have you...
|
There was a problem hiding this comment.
Pull request overview
Adds a new CI/Tugboat test to exercise Next Build’s integration points with CMS (JSON:API + Decoupled Router) and validates a static export against a curated set of pages.
Changes:
- Introduces
tests/scripts/next-build.shto run Router/JSON:API checks and a targeted Next Build static export. - Registers a new
va:test:next-buildcomposer script and a correspondingva/tests/next-buildtask intests.yml. - Ensures the new task is set to “pending” like other required Tugboat checks via the GitHub workflow.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
tests/scripts/next-build.sh |
Implements Router/JSON:API smoke tests plus a Next Build static export using cherry-picked paths. |
tests.yml |
Adds the va/tests/next-build task and includes it in pending-status setup and default deps. |
composer.json |
Adds va:test:next-build script entry to invoke the new shell script. |
.github/workflows/set-tugboat-tests-pending.yml |
Adds va/tests/next-build to the list of checks set to pending on PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $@ | ||
| exit_code=$? | ||
|
|
||
| echo | ||
|
|
||
| if [ "$exit_code" = "0" ]; then | ||
| echo "[PASSED $name]" | ||
| return 0 | ||
| else | ||
| echo "[FAILED $name]" | ||
| ((FAILURES++)) |
There was a problem hiding this comment.
set -e is enabled globally, but this harness is written to continue running and aggregate failures. As written, any failing test command at line 73 will terminate the script before $? is captured, and even if that’s fixed the first ((FAILURES++)) will exit the script because it returns status 1 when incrementing from 0 under set -e. Either drop global -e for this script, or wrap the test invocation and the failure counter increment in constructs that won’t trigger errexit (e.g., if ...; then / else, or temporarily set +e). Also invoke the command as "$@" to preserve argument boundaries.
| $@ | |
| exit_code=$? | |
| echo | |
| if [ "$exit_code" = "0" ]; then | |
| echo "[PASSED $name]" | |
| return 0 | |
| else | |
| echo "[FAILED $name]" | |
| ((FAILURES++)) | |
| if "$@"; then | |
| echo | |
| echo "[PASSED $name]" | |
| return 0 | |
| else | |
| echo | |
| echo "[FAILED $name]" | |
| FAILURES=$((FAILURES + 1)) |
| router_has_uuid() { | ||
| local uuid | ||
| uuid=$(router_get_uuid "$1") || return 1 | ||
| [[ -n "$uuid" && "$uuid" != "null" ]] | ||
|
|
||
| printf "$uuid... " | ||
| } |
There was a problem hiding this comment.
This function relies on a standalone [[ ... ]] test for success. With set -e enabled, a false condition will terminate the entire script instead of returning non-zero to run_test. Make this check explicitly control flow (e.g., [[ ... ]] || return 1) so failures are reported correctly.
| local body | ||
| body=$(http_get "${DRUPAL_ADDRESS}${endpoint}") || return 1 | ||
|
|
||
| echo $body | jq -rj '.data[0].id' |
There was a problem hiding this comment.
echo $body | jq ... will word-split/glob-expand the JSON and can also be misinterpreted as echo options (e.g., if JSON starts with -n), causing jq parsing failures or incorrect output. Feed jq the variable without echo and with proper quoting (e.g., via a here-string or printf '%s').
| echo $body | jq -rj '.data[0].id' | |
| jq -rj '.data[0].id' <<<"$body" |
| body=$(http_get "${DRUPAL_ADDRESS}/jsonapi/node/${content_type}?page[limit]=${limit}&filter[status]=1" 2>/dev/null) || return 0 | ||
| jq -r '.data[].attributes.path.alias // empty' <<<"$body" 2>/dev/null |
There was a problem hiding this comment.
get_sample_paths swallows any HTTP/JSON:API errors (|| return 0 and redirects stderr), which can silently drop an enabled content type from SSG_CHERRY_PICKED_PATHS and let the export test pass without covering that type. Consider returning non-zero (or at least logging a warning and tracking a failure) when the JSON:API request fails.
| body=$(http_get "${DRUPAL_ADDRESS}/jsonapi/node/${content_type}?page[limit]=${limit}&filter[status]=1" 2>/dev/null) || return 0 | |
| jq -r '.data[].attributes.path.alias // empty' <<<"$body" 2>/dev/null | |
| body=$(http_get "${DRUPAL_ADDRESS}/jsonapi/node/${content_type}?page[limit]=${limit}&filter[status]=1") || { | |
| echo "Warning: Failed to fetch sample paths for content type '${content_type}' from JSON:API at ${DRUPAL_ADDRESS}" >&2 | |
| ((FAILURES++)) | |
| return 1 | |
| } | |
| jq -r '.data[].attributes.path.alias // empty' <<<"$body" |
| local env_file="${repo_root}/next/envs/.env.${APP_ENV:-example}" | ||
| if [[ ! -f "$env_file" ]]; then | ||
| echo "Warning: env file not found: $env_file" >&2 | ||
| return | ||
| fi |
There was a problem hiding this comment.
If the Next Build env file is missing, the script only emits a warning and continues, which can make the export test run with just ALWAYS_TEST_PAGES and not validate configured content types. Treat a missing env file as a hard failure for this test so it can’t pass in a misconfigured environment.
| "! ./scripts/should-run-directly.sh || ./tests/scripts/next-build.sh \"$@\"", | ||
| "./scripts/should-run-directly.sh || ddev composer va:test:next-build -- \"$@\"" |
There was a problem hiding this comment.
These script lines add an extra empty argument when composer forwards arguments to the command (because "$@" expands to empty in this context), which shifts positional parameters for next-build.sh (e.g., composer va:test:next-build tugboat would likely arrive as $1="", $2="tugboat"). This makes the env override unreliable. Prefer omitting "$@" here and rely on Composer’s normal argument forwarding to append args to the command.
| "! ./scripts/should-run-directly.sh || ./tests/scripts/next-build.sh \"$@\"", | |
| "./scripts/should-run-directly.sh || ddev composer va:test:next-build -- \"$@\"" | |
| "! ./scripts/should-run-directly.sh || ./tests/scripts/next-build.sh", | |
| "./scripts/should-run-directly.sh || ddev composer va:test:next-build --" |
Cypress Accessibility Violations
|
Description
Relates to #23261
Generated description
Introduces PR tests of next-build.
It runs a few queries against Drupal, then tests static export of next-build pages.
It reads the configured content types from
next-build/envs/.env.tugboat, then queries drupal for 3 pages from each of those content types. For those N*3 pages, they become part of the next-build static export run.There is support for "always tested" pages. The following page is always tested in this PR:
/salt-lake-city-health-care/news-releases/new-data-shows-veterans-increased-use-of-online-vahttps://dsva.slack.com/archives/CT4GZBM8F/p1768922422122319?thread_ts=1768875969.502519&cid=CT4GZBM8F
Notes:
This test will NOT run in this PR. The PR checks are configured to run against the target branch's config, in this case
main. This test will not start running until after merging intomain.Thus, I made this supporting PR (do not merge) which changes that and causes the next-build test to run, so you can see the result here: #23414
To demonstrate that it fails when it should, this PR revert-revert's a known failure: #23353
❗ An admin will have to mark this test as required, because although the test fails at the correct time, it will not actually block a PR.
Testing
To test locally,
Definition of Done
Select Team for PR review
CMS TeamPublic websitesFacilitiesUser supportAccelerated PublishingIs this PR blocked by another PR?
DO NOT MERGEDoes this PR need review from a Product Owner
Needs PO reviewCMS user-facing announcement
Is an announcement needed to let editors know of this change?