Conversation
Contributor
📝 WalkthroughWalkthroughAdds testing infrastructure: Playwright E2E config and suites, Vitest unit tests for utilities, a translation validation script, package.json test scripts/devDependencies, testids on components, and README testing docs; also removes NewsletterForm from knip ignore and minor doc whitespace fix. Changes
Sequence Diagram(s)mermaid Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/utils/safe-execute.test.ts`:
- Around line 28-35: The test name claims to verify logger behavior for
safeExecute but only checks the returned result; either change the test to
assert the logger call or rename the test to match its assertion. To verify
logging, import or mock the logger used by safeExecute (spy on logger.error),
call safeExecute(throwFn, null, 'Custom error message'), and assert logger.error
was called with the custom message and/or the thrown Error; alternatively,
rename the test string to something like "should return fallback when fn throws
with custom error message" to reflect the current assertion. Ensure you
reference the safeExecute function and the throwFn used in the test when making
changes.
In `@tests/e2e/locale-switching.spec.ts`:
- Around line 32-44: The test "should persist locale via cookie after switch"
currently only asserts the cookie contains a valid locale; instead determine the
expected locale after clicking the localeButton and assert localeCookie.value
equals that expected value. After clicking localeButton (variable localeButton)
and awaiting networkidle, derive the expectedLocale from the UI change you
trigger (e.g., check the new label/text/aria of localeButton or another element
that reflects the selected language) and then replace the loose assertion on
localeCookie.value with a strict equality check (localeCookie?.value ===
expectedLocale) using context.cookies() and localeCookie as currently
implemented.
- Around line 16-30: The test "should switch from English to Czech" currently
only checks post-click state and uses a loose OR that can pass if page starts in
Czech; first assert the initial locale is English by inspecting htmlLang
(htmlLang === 'en') or bodyText includes English labels like 'Home' or
'Projects' before clicking the localeButton, then click localeButton, wait for
networkidle, and assert the locale actually transitioned by checking htmlLang
=== 'cs' and bodyText includes Czech strings like 'Domů' and/or 'Projekty'
(replace the single OR assertion with two explicit expects: one for initial
state and one for post-click state); refer to the localeButton variable,
htmlLang and bodyText locators and the test block name to find the code to
change.
In `@tests/e2e/newsletter.spec.ts`:
- Around line 11-17: The test 'should display newsletter form on homepage' uses
hardcoded English strings in its selectors (getByRole heading name 'Subscribe to
Our Newsletter', getByPlaceholder 'Enter your email', button name 'Subscribe')
which breaks under other locales; update the NewsletterForm (or template) to add
stable data-testid attributes for the heading, input and button (e.g.,
data-testid="newsletter-heading", "newsletter-email", "newsletter-submit") and
then change the test to use page.getByTestId(...) for those elements, or
alternatively replace the string matchers with locale-agnostic regexes (e.g.,
/Subscribe|Odebírat/i) to cover both English and Czech; ensure selectors
reference the new test IDs or regexes in the test named 'should display
newsletter form on homepage'.
🧹 Nitpick comments (3)
src/utils/safe-execute.test.ts (1)
45-50: Test name is confusing.The phrase "async-like synchronous return values" is unclear. The test verifies that object references are preserved (identity check via
toBe), which is straightforward. Consider a clearer name like "should preserve object reference on success".Suggested rename
- it('should propagate async-like synchronous return values', () => { + it('should preserve object reference on success', () => { const obj = { key: 'value' }; const result = safeExecute(() => obj, {}); expect(result).toBe(obj); });tests/e2e/theme-switching.spec.ts (1)
30-39: Consider asserting the specific theme value.Similar to the locale tests, the assertion at line 38 only verifies the theme is valid ('light' or 'dark'), not that it reflects the change from clicking the toggle. However, since the toggle behavior is verified in the adjacent test (lines 16-28), this is acceptable for a persistence check.
Optional: more precise assertion
test('should persist theme preference in localStorage', async ({ page }) => { await page.goto('/'); + const hadDarkBefore = await page.evaluate(() => document.documentElement.classList.contains('dark')); const themeButton = page.getByRole('button', { name: /Toggle theme|Přepnout motiv/i }); await themeButton.click(); const theme = await page.evaluate(() => localStorage.getItem('theme')); - expect(theme === 'light' || theme === 'dark').toBeTruthy(); + expect(theme).toBe(hadDarkBefore ? 'light' : 'dark'); });src/utils/dom.test.ts (1)
49-55: Consider consolidating with the first test case.This test duplicates the localStorage assertion from
'should add dark class when not present'. While explicit persistence tests can improve clarity, this specific test doesn't add new coverage.That said, keeping it separate does make the test suite more readable for someone scanning test names, so this is a minor observation rather than a blocker.
Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-authored-by: Cursor <cursoragent@cursor.com>
…e sizes - Add AVIF support documentation (Sharp supports it; use format="avif" on Image) - Add priority prop and decoding attribute to ResponsiveImage (sync for LCP, async for lazy) - Change GoToTop hydration from client:idle to client:visible - Fine-tune getResponsiveSizes for mobile/tablet (100vw, 95vw, 85vw, 75vw breakpoints) - Document image strategy in ai-rules (AVIF, priority, decoding, responsive sizes) Closes AL-217 Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-authored-by: Cursor <cursoragent@cursor.com>
- Add safe-execute.test.ts and dom.test.ts unit tests - Add E2E tests for newsletter, locale switching, theme switching - Add validate-translations.ts script for key completeness - Add NewsletterForm to Footer for newsletter E2E flow - Add playwright.config.ts with webServer for preview - Update package.json with test:e2e and validate:translations scripts - Update README with testing documentation - Remove NewsletterForm from knip ignore (now used in Footer) Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-authored-by: Cursor <cursoragent@cursor.com>
vancura
force-pushed
the
al-216-testing-unit-and-e2e-coverage-for-critical-flows
branch
from
4df1fd7 to
ab6e27c
Compare
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
- Import additional testing utilities from Vitest - Add console error spying to capture and verify error messages during tests - Update tests to check for correct error logging when exceptions are thrown Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Vaclav Vancura <commit@vancura.dev>
- Add logger verification to safe-execute test by mocking console.error and asserting it's called with custom error message and Error object - Add initial state verification to locale-switching E2E test to ensure page starts in English before clicking locale switcher - Replace loose OR assertion with explicit pre-click and post-click state checks to verify actual locale transition from EN to CS - Use separate variables for before/after state to improve test clarity These changes ensure tests verify actual behavior rather than just checking post-action state, making them more robust and easier to debug. Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Vaclav Vancura <commit@vancura.dev>
- Add console.error spy to safe-execute test to verify logger is called with custom error message and Error object when function throws - Add initial English state verification to locale-switching E2E test before clicking switcher to ensure proper transition baseline - Replace loose OR assertion with explicit before/after state checks to verify actual EN→CS locale transition occurred - Update cookie persistence test to derive expected locale from html lang attribute and assert exact match instead of loose OR check These changes ensure tests verify complete behavior flows rather than just final states, making them more robust and better at catching regressions in error logging, locale transitions, and cookie sync. Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Vaclav Vancura <commit@vancura.dev>
- Introduced 'data-testid' attributes in Button and NewsletterForm components to enhance accessibility for testing. - Updated e2e tests to utilize these attributes for selecting elements, improving the reliability of the tests. These changes facilitate better integration with testing frameworks and ensure that UI components can be easily targeted in automated tests. Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Vaclav Vancura <commit@vancura.dev>
…nd-e2e-coverage-for-critical-flows # Conflicts: # ai-rules/astro-svelte.mdc # src/components/astro/Footer.astro # src/components/svelte/NewsletterForm.svelte Signed-off-by: Vaclav Vancura <commit@vancura.dev>
Signed-off-by: Vaclav Vancura <commit@vancura.dev>
… and dynamic pages - Removed the NewsletterForm component from the Footer.astro file. - Added the NewsletterForm component to the NewsPostLayout.astro file and dynamic pages for better integration and visibility. Signed-off-by: Vaclav Vancura <commit@vancura.dev>
Signed-off-by: Vaclav Vancura <commit@vancura.dev>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/dom.test.ts`:
- Around line 6-28: The tests mutate document.documentElement classes but don't
restore them; capture the initial className in beforeEach (e.g., save
document.documentElement.className into a variable alongside
mockDocumentElement) and then restore it in afterEach before calling
vi.unstubAllGlobals by setting document.documentElement.className back to the
saved value; update the existing beforeEach/afterEach that reference
mockDocumentElement and vi.unstubAllGlobals to use this saved initialClass so
the `dark` class (or any other mutation) doesn't leak between tests.
- Added logic to preserve the initial class name of the document element in the dom tests. - Ensured that the class name is reset after each test to maintain test isolation. Signed-off-by: Vaclav Vancura <commit@vancura.dev> Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
vancura
deleted the
al-216-testing-unit-and-e2e-coverage-for-critical-flows
branch
This was referenced Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linear
AL-216: Testing: Unit and E2E Coverage for Critical Flows
Summary
Adds comprehensive testing infrastructure for the Ambilab website:
Changes
How to run
pnpm test:runpnpm build && pnpm test:e2epnpm validate:translationsAcceptance criteria
Made with Cursor
Testing Infrastructure Implementation
This PR adds comprehensive testing support: Vitest for unit tests, Playwright for E2E tests, and a translation completeness validator.
Test Configuration
Unit Tests
End-to-End Tests
Translation Validation
Component & Integration Changes to Support Tests
Package & Tooling Updates
Documentation
How to run
Acceptance criteria (covered)