Skip to content

Commit 0eae20a

Browse files
committed
test: address PR review feedback on doc site test refactor
- playwright.config.ts: restore build-then-preview in webServer so bare `playwright test` and IDE run buttons are self-contained again. Playwright starts webServer before loading spec files, so this also supplies the sitemap the a11y project reads at collection time; the test scripts no longer build, avoiding a double build. - tests/chart-api-mock.ts: extract the chart API mock shared by both suites, with every route anchored to the API host. Bare path patterns also matched the docs site's own /indicators page and answered its document request with JSON. - tests/a11y.spec.ts: replace the discouraged `networkidle` wait with web-first assertions on the content root and on each chart reaching a terminal state, and serve charts from fixtures rather than the live API. - test-links.sh: install the pinned html-proofer when Ruby is present, so HTMLPROOFER_VERSION lives only in the script; the workflow drops its own gem install rather than duplicating the pin. - config.mts: state the collapsibility trade-off for the "Other price patterns" sidebar group explicitly. - test-website-a11y.yml: upload-artifact v4 -> v7, matching the repo. - migration/v3.md: restore the PRINCIPLES.md rationale link and the ToKeltner()/ToStdDevChannels() remediation pointers.
1 parent a686375 commit 0eae20a

10 files changed

Lines changed: 135 additions & 74 deletions

File tree

.github/workflows/test-website-a11y.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ jobs:
6363

6464
- name: Upload report
6565
if: ${{ !cancelled() }}
66-
uses: actions/upload-artifact@v4
66+
uses: actions/upload-artifact@v7
6767
with:
6868
name: a11y-report
6969
path: docs/playwright-report

.github/workflows/test-website-links.yml

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,10 @@ jobs:
5050
- name: Install pnpm packages
5151
run: pnpm install --frozen-lockfile
5252

53-
- name: Install htmlproofer
54-
run: gem install html-proofer -v 5.0.10
55-
56-
# The script builds the site and owns the ignore lists and retry policy,
57-
# so this check cannot drift from what contributors run locally. It uses
58-
# the htmlproofer installed above rather than its container fallback.
53+
# The script owns everything about the check — the build, the pinned
54+
# html-proofer version, the ignore lists and the retry policy — so CI
55+
# cannot drift from what contributors run locally. Ruby is on PATH from
56+
# the setup step above, so the script installs and uses the gem natively
57+
# rather than falling back to its container path.
5958
- name: Test for broken URLs
6059
run: pnpm run test:links

docs/.vitepress/config.mts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -284,12 +284,22 @@ export default defineConfig({
284284
{ text: 'Doji', link: '/indicators/doji' },
285285
{ text: 'Marubozu', link: '/indicators/marubozu' },
286286
{
287-
// No `collapsed` key on purpose: a collapsible group without its
288-
// own `link` renders `role="button"` on the group header AND on
289-
// the caret inside it, which axe flags as nested-interactive
290-
// (WCAG 4.1.2) on every page this group is expanded for. Omitting
291-
// `collapsed` makes it a plain always-open heading with no
292-
// button, keeping the grouping without the nested controls.
287+
// No `collapsed` key on purpose, and note the trade-off: this
288+
// group is now permanently non-collapsible, not merely open by
289+
// default. VitePress treats `collapsed != null` as "collapsible",
290+
// so any value renders a caret with `role="button"` inside a
291+
// header that is itself `role="button"` (the header takes that
292+
// role because this group has no `link` of its own). axe flags
293+
// that nesting as WCAG 4.1.2 nested-interactive on every page the
294+
// group is expanded for. Dropping the key removes both controls.
295+
//
296+
// Intended: these two entries should simply be visible whenever
297+
// the parent "Candlestick patterns" group is open. Collapsing
298+
// them independently was never the goal, and the header had no
299+
// page behind it to make the affordance meaningful. To restore
300+
// collapsibility without the violation, this group would need its
301+
// own `link` — then the header renders as an <a> beside a
302+
// separate caret button instead of nesting two buttons.
293303
text: 'Other price patterns',
294304
items: [
295305
{ text: 'Pivots', link: '/indicators/pivots' },

docs/.vitepress/test-links.sh

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,16 +67,25 @@ run_docker() {
6767

6868
# Probe by running it, not with `command -v`: Windows ships an App Execution
6969
# Alias shim named `htmlproofer` that resolves on PATH but fails to launch, and
70-
# a PATH-only check would pick that over a working Docker fallback.
70+
# a PATH-only check would pick that over a working install path.
71+
#
72+
# When Ruby is present but html-proofer is not, install it here rather than in
73+
# the caller. That keeps HTMLPROOFER_VERSION above as the only place the pin
74+
# lives — CI just runs this script instead of installing the gem itself.
75+
if ! htmlproofer --version >/dev/null 2>&1 && command -v gem >/dev/null 2>&1; then
76+
echo "Installing html-proofer ${HTMLPROOFER_VERSION}..."
77+
gem install --no-document html-proofer -v "${HTMLPROOFER_VERSION}" || true
78+
fi
79+
7180
if htmlproofer --version >/dev/null 2>&1; then
7281
echo "Using native htmlproofer: $(htmlproofer --version 2>&1 | head -n 1)"
7382
run_check() { run_native; }
7483
elif command -v docker >/dev/null 2>&1; then
7584
echo "No native htmlproofer; using ${RUBY_IMAGE} with html-proofer ${HTMLPROOFER_VERSION}"
7685
run_check() { run_docker; }
7786
else
78-
echo "Error: needs either a 'htmlproofer' binary on PATH or Docker." >&2
79-
echo " gem install html-proofer -v ${HTMLPROOFER_VERSION}" >&2
87+
echo "Error: needs Ruby (to install html-proofer) or Docker." >&2
88+
echo " gem install --no-document html-proofer -v ${HTMLPROOFER_VERSION}" >&2
8089
exit 1
8190
fi
8291

docs/migration/v3.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,13 @@ Most of the deprecated v2 syntax has been shimmed in library version `3.0` with
2222

2323
**The rest rejected calls that v2 accepted** — these failed loudly at build or run time, never with quietly wrong values:
2424

25-
- **`GetKeltner()` rejected a fractional multiplier.** It declared `int multiplier` where v2 declares `double`, so `quotes.GetKeltner(20, 2.5, 10)` would not compile. Do not round the multiplier to work around it; that silently changes your channel widths.
26-
- **`GetStdDevChannels()` rejected a null lookback** through its tuple overload, declaring `int lookbackPeriods` where v2 declares `int?`, whose `null` means "use the whole series". Do not substitute a fixed lookback; that computes a different channel.
25+
- **`GetKeltner()` rejected a fractional multiplier.** It declared `int multiplier` where v2 declares `double`, so `quotes.GetKeltner(20, 2.5, 10)` would not compile. Do not round the multiplier to work around it; that silently changes your channel widths. Upgrade to `3.0.1`, or move to `ToKeltner()`, which always accepted these values.
26+
- **`GetStdDevChannels()` rejected a null lookback** through its tuple overload, declaring `int lookbackPeriods` where v2 declares `int?`, whose `null` means "use the whole series". Do not substitute a fixed lookback; that computes a different channel. The bar overload of the same method still accepts `null` — and its own deprecation message points you there — so the two forms disagreed. Upgrade to `3.0.1`, or move to `ToStdDevChannels()`.
2727
- **`GetPrs()` with no `lookbackPeriods` threw.** The unspecified lookback mapped to `0`, which validation rejects, so the shim's own default raised `ArgumentOutOfRangeException`. `3.0.1` computes with a null `PrsPercent`, as v2 did.
2828
- **`GetPrs()` and `GetRoc()` tuple overloads silently ignored `smaPeriods`.** They still accept and discard the argument — v3 has no `PrsSma`/`RocSma` field — but as of `3.0.1` their deprecation messages say so.
2929

30+
These ship as bug fixes rather than breaking changes because the shipped behavior never matched its own documentation — see the correctness principles on [correcting a defect versus changing intended behavior](https://github.com/facioquo/stock-indicators-dotnet/blob/main/docs/PRINCIPLES.md).
31+
3032
None of these fixes reach you by dropping the `3.0.1` assembly next to code compiled against `3.0.0`**recompile.** `GetKeltner()` and `GetStdDevChannels()` changed parameter types, so existing call sites no longer bind to a method that exists. And `GetPvo()`'s corrected defaults are baked into the _caller's_ IL at compile time, so an argument-less call keeps computing `9`/`12` until it is rebuilt. Recompiling from source requires no edits.
3133
:::
3234

docs/package.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@
1212
"docs:dev": "vitepress dev",
1313
"docs:build": "vitepress build",
1414
"docs:preview": "vitepress preview",
15-
"test": "vitepress build && playwright test",
16-
"test:charts": "vitepress build && playwright test --project=charts",
17-
"test:a11y": "vitepress build && playwright test --project=a11y",
15+
"test": "playwright test",
16+
"test:charts": "playwright test --project=charts",
17+
"test:a11y": "playwright test --project=a11y",
1818
"test:links": "bash .vitepress/test-links.sh"
1919
},
2020
"devDependencies": {

docs/playwright.config.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,18 @@ import { defineConfig, devices } from '@playwright/test'
77
* charts - visual chart rendering, mocked against static fixture data
88
* a11y - axe-core WCAG 2.1 A/AA scan of every page in the sitemap
99
*
10-
* Run via the package scripts, which build the site first:
10+
* Run via the package scripts, or drive Playwright directly:
1111
* pnpm run test # both projects
1212
* pnpm run test:charts
1313
* pnpm run test:a11y
14+
* pnpm exec playwright test --ui
1415
*
15-
* The build must precede the run because the a11y project enumerates its
16-
* pages from `.vitepress/dist/sitemap.xml` while collecting tests, before
17-
* `webServer` starts. That is also why `webServer` only previews.
16+
* `webServer` builds before previewing so every entry point is self-contained
17+
* — an IDE run button or a bare `playwright test` gets a fresh build rather
18+
* than previewing stale output. Playwright starts `webServer` before it loads
19+
* spec files, so the build is also what puts `.vitepress/dist/sitemap.xml` in
20+
* place for the a11y project, which enumerates its pages from it at
21+
* collection time.
1822
*/
1923
export default defineConfig({
2024
testDir: './tests',
@@ -48,7 +52,7 @@ export default defineConfig({
4852
],
4953

5054
webServer: {
51-
command: 'pnpm run docs:preview',
55+
command: 'pnpm run docs:build && pnpm run docs:preview',
5256
url: 'http://localhost:4173',
5357
reuseExistingServer: !process.env.CI,
5458
timeout: 180_000,

docs/tests/a11y.spec.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import { readFileSync } from 'fs'
44
import { fileURLToPath } from 'url'
55
import { dirname, join } from 'path'
66

7+
import { mockStockChartsApi, CHART_TERMINAL_SELECTOR } from './chart-api-mock'
8+
79
const __dirname = dirname(fileURLToPath(import.meta.url))
810
const SITEMAP = join(__dirname, '../.vitepress/dist/sitemap.xml')
911

@@ -84,11 +86,25 @@ for (const path of PAGES) {
8486
test(`a11y - ${path}`, async ({ page }) => {
8587
const analyticsAttempts = await blockAnalytics(page)
8688

89+
// Serve charts from the same fixtures the chart suite uses. Without this
90+
// the scan waits on the live API — slow, and it would only ever scan the
91+
// failure UI rather than a rendered chart.
92+
await mockStockChartsApi(page)
93+
8794
await page.goto(path, { waitUntil: 'domcontentloaded' })
8895

89-
// Charts mount behind ClientOnly and paint after hydration; waiting for the
90-
// network to settle keeps the scan from racing late-inserted content.
91-
await page.waitForLoadState('networkidle')
96+
// Web-first waits rather than `networkidle`, which Playwright discourages
97+
// for tests. The content root proves the page rendered; charts mount behind
98+
// ClientOnly after hydration, so any chart on the page must also reach a
99+
// terminal state before axe looks at it.
100+
await expect(page.locator('.VPContent')).toBeVisible()
101+
102+
const charts = page.locator('[data-testid$="-root"]')
103+
for (let i = 0; i < (await charts.count()); i++) {
104+
await expect(
105+
charts.nth(i).locator(CHART_TERMINAL_SELECTOR).first()
106+
).toBeVisible({ timeout: 20_000 })
107+
}
92108

93109
const { violations } = await new AxeBuilder({ page })
94110
.withTags(WCAG_TAGS)

docs/tests/chart-api-mock.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import type { Page, Route } from '@playwright/test'
2+
import { readFileSync } from 'fs'
3+
import { fileURLToPath } from 'url'
4+
import { dirname, join } from 'path'
5+
6+
const __dirname = dirname(fileURLToPath(import.meta.url))
7+
const FIXTURES = join(__dirname, '../.vitepress/public/data/chart-api')
8+
9+
// Static fixture data loaded once
10+
const quotesJson = readFileSync(join(FIXTURES, 'quotes.json'), 'utf8')
11+
const indicatorsJson = readFileSync(join(FIXTURES, 'indicators.json'), 'utf8')
12+
const smaJson = readFileSync(join(FIXTURES, 'sma.json'), 'utf8')
13+
const rsiJson = readFileSync(join(FIXTURES, 'rsi.json'), 'utf8')
14+
15+
/**
16+
* Intercept all stock-charts API requests and respond with static fixture data,
17+
* so suites are hermetic and never depend on the live API.
18+
*
19+
* The API serves indicator data from per-indicator endpoints keyed by UIID
20+
* (e.g. `/SMA/`, `/RSI/`, `/MACD/`) — NOT `/indicators/<name>`. The listings
21+
* fixture (`indicators.json`) carries those absolute endpoints, so the client
22+
* requests `/<UIID>/` and the routes below must match that shape.
23+
*
24+
* Every pattern is anchored to the API host. Bare path patterns would also
25+
* match same-named routes on the docs site itself — `/indicators` is a real
26+
* page — and would answer that document request with JSON.
27+
*/
28+
const API = 'charts-api\\.stockindicators\\.dev'
29+
30+
export async function mockStockChartsApi(page: Page): Promise<void> {
31+
// Routes are matched LIFO (last-registered = highest priority). This catch-all
32+
// is registered first, so every specific route below shadows it. Any indicator
33+
// endpoint we don't explicitly fixture returns an empty array → the chart
34+
// reaches the (tolerated) empty state instead of touching the network.
35+
await page.route(new RegExp(`${API}/.+`), (route: Route) =>
36+
route.fulfill({ contentType: 'application/json', body: '[]' })
37+
)
38+
39+
await page.route(new RegExp(`${API}/quotes(?:\\?|$)`), (route: Route) =>
40+
route.fulfill({ contentType: 'application/json', body: quotesJson })
41+
)
42+
43+
await page.route(new RegExp(`${API}/indicators(?:\\?|$)`), (route: Route) =>
44+
route.fulfill({ contentType: 'application/json', body: indicatorsJson })
45+
)
46+
47+
// SMA indicator data — endpoint is `/SMA/?lookbackPeriods=...`
48+
await page.route(new RegExp(`${API}/SMA/`, 'i'), (route: Route) =>
49+
route.fulfill({ contentType: 'application/json', body: smaJson })
50+
)
51+
52+
// RSI indicator data — endpoint is `/RSI/?lookbackPeriods=...`
53+
await page.route(new RegExp(`${API}/RSI/`, 'i'), (route: Route) =>
54+
route.fulfill({ contentType: 'application/json', body: rsiJson })
55+
)
56+
}
57+
58+
/**
59+
* Selector matching any terminal state of a chart: rendered, empty, or errored.
60+
* Waiting on this is the web-first way to know a chart has stopped changing.
61+
*/
62+
export const CHART_TERMINAL_SELECTOR =
63+
'[data-testid$="-overlay-canvas"], [data-testid$="-empty"], [data-testid$="-error"]'

docs/tests/charts.spec.ts

Lines changed: 3 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,55 +1,13 @@
1-
import { test, expect, type Page, type Route } from '@playwright/test'
1+
import { test, expect, type Page } from '@playwright/test'
22
import { readdirSync, readFileSync } from 'fs'
33
import { fileURLToPath } from 'url'
44
import { dirname, join } from 'path'
55

66
import { getTestIdPrefix } from '@facioquo/indy-charts/vue'
77

8-
const __dirname = dirname(fileURLToPath(import.meta.url))
9-
const FIXTURES = join(__dirname, '../.vitepress/public/data/chart-api')
10-
11-
// Static fixture data loaded once
12-
const quotesJson = readFileSync(join(FIXTURES, 'quotes.json'), 'utf8')
13-
const indicatorsJson = readFileSync(join(FIXTURES, 'indicators.json'), 'utf8')
14-
const smaJson = readFileSync(join(FIXTURES, 'sma.json'), 'utf8')
15-
const rsiJson = readFileSync(join(FIXTURES, 'rsi.json'), 'utf8')
8+
import { mockStockChartsApi } from './chart-api-mock'
169

17-
/**
18-
* Intercept all stock-charts API requests and respond with static fixture data,
19-
* so the suite is hermetic and never depends on the live API.
20-
*
21-
* The API serves indicator data from per-indicator endpoints keyed by UIID
22-
* (e.g. `/SMA/`, `/RSI/`, `/MACD/`) — NOT `/indicators/<name>`. The listings
23-
* fixture (`indicators.json`) carries those absolute endpoints, so the client
24-
* requests `/<UIID>/` and the routes below must match that shape.
25-
*/
26-
async function mockStockChartsApi(page: Page): Promise<void> {
27-
// Routes are matched LIFO (last-registered = highest priority). This catch-all
28-
// is registered first, so every specific route below shadows it. Any indicator
29-
// endpoint we don't explicitly fixture returns an empty array → the chart
30-
// reaches the (tolerated) empty state instead of touching the network.
31-
await page.route(/charts-api\.stockindicators\.dev\/.+/, (route: Route) =>
32-
route.fulfill({ contentType: 'application/json', body: '[]' })
33-
)
34-
35-
await page.route(/\/quotes(?:\?|$)/, (route: Route) =>
36-
route.fulfill({ contentType: 'application/json', body: quotesJson })
37-
)
38-
39-
await page.route(/\/indicators(?:\?|$)/, (route: Route) =>
40-
route.fulfill({ contentType: 'application/json', body: indicatorsJson })
41-
)
42-
43-
// SMA indicator data — endpoint is `/SMA/?lookbackPeriods=...`
44-
await page.route(/\/SMA\//i, (route: Route) =>
45-
route.fulfill({ contentType: 'application/json', body: smaJson })
46-
)
47-
48-
// RSI indicator data — endpoint is `/RSI/?lookbackPeriods=...`
49-
await page.route(/\/RSI\//i, (route: Route) =>
50-
route.fulfill({ contentType: 'application/json', body: rsiJson })
51-
)
52-
}
10+
const __dirname = dirname(fileURLToPath(import.meta.url))
5311

5412
/**
5513
* Wait for a chart to reach a terminal state (ready, empty, or error).

0 commit comments

Comments
 (0)