-
Notifications
You must be signed in to change notification settings - Fork 1
adding more tests #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
18a6faf
ecb5a9a
18d4b4b
2699b8f
16c2a2c
701ebcf
c77fded
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,4 +21,304 @@ test.describe('base tests', () => { | |||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // TODO: add tests for each of the possible parameters | ||||||||||||||||||||||||||||||||||||||||||||
| test.describe('component parameters', () => { | ||||||||||||||||||||||||||||||||||||||||||||
| test.beforeEach(async ({ page }) => { | ||||||||||||||||||||||||||||||||||||||||||||
| await page.goto(fileUrl); | ||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| test.describe('prefix and suffix', () => { | ||||||||||||||||||||||||||||||||||||||||||||
| test('renders with default prefix $ and suffix USD', async ({ page }) => { | ||||||||||||||||||||||||||||||||||||||||||||
| const currencyInput = await page.locator('#currency-input'); | ||||||||||||||||||||||||||||||||||||||||||||
| await expect(currencyInput).toHaveValue('$0.00 USD'); | ||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+33
|
||||||||||||||||||||||||||||||||||||||||||||
| test('renders with default prefix $ and suffix USD', async ({ page }) => { | |
| const currencyInput = await page.locator('#currency-input'); | |
| await expect(currencyInput).toHaveValue('$0.00 USD'); | |
| }); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable currencyInput.
| const currencyInput = page.locator('#currency-input'); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable currencyInput.
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded timeout of 100ms is used here and in multiple other tests throughout this file (lines 123, 148, 170, 238). These arbitrary timeouts can lead to flaky tests and don't follow Playwright best practices. Consider using Playwright's auto-waiting capabilities or explicit wait conditions instead of fixed timeouts.
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using page.waitForTimeout is an anti-pattern in Playwright tests and can lead to flaky tests. Instead of arbitrary timeouts, use explicit waits for specific conditions, such as waiting for the input to have a certain attribute or value after clicking the apply button. Consider using expect with auto-retrying assertions or page.waitForFunction to wait for the state to update.
| await page.waitForTimeout(100); | |
| await expect(decimalInput).toHaveValue(','); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded timeout of 100ms is used here and in multiple other tests throughout this file (lines 148, 170, 238). These arbitrary timeouts can lead to flaky tests and don't follow Playwright best practices. Consider using Playwright's auto-waiting capabilities or explicit wait conditions instead of fixed timeouts.
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded timeout of 100ms is used here and in multiple other tests throughout this file (lines 170, 238). These arbitrary timeouts can lead to flaky tests and don't follow Playwright best practices. Consider using Playwright's auto-waiting capabilities or explicit wait conditions instead of fixed timeouts.
| await page.waitForTimeout(100); | |
| const currencyInput = page.locator('#currency-input'); | |
| const currencyInput = page.locator('#currency-input'); | |
| await expect(currencyInput).toHaveValue(''); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded timeout of 100ms is used here and in another test (line 238). These arbitrary timeouts can lead to flaky tests and don't follow Playwright best practices. Consider using Playwright's auto-waiting capabilities or explicit wait conditions instead of fixed timeouts.
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using page.waitForTimeout is an anti-pattern in Playwright tests and can lead to flaky tests. Instead of arbitrary timeouts, use explicit waits for specific conditions, such as waiting for the input to have a certain attribute or value after clicking the apply button. Consider using expect with auto-retrying assertions or page.waitForFunction to wait for the state to update.
| await page.waitForTimeout(100); | |
| const currencyInput = page.locator('#currency-input'); | |
| const currencyInput = page.locator('#currency-input'); | |
| await expect(currencyInput).toBeEnabled(); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests don't actually verify the allowEmpty functionality because they never enable allowEmpty=true. Both tests verify default behavior where allowEmpty is false. To properly test allowEmpty, add tests that enable the setting and verify that the input can remain empty without defaulting to "$0.00 USD".
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded timeout of 100ms is used here. These arbitrary timeouts can lead to flaky tests and don't follow Playwright best practices. Consider using Playwright's auto-waiting capabilities or explicit wait conditions instead of fixed timeouts.
| await page.waitForTimeout(100); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using page.waitForTimeout is an anti-pattern in Playwright tests and can lead to flaky tests. Instead of arbitrary timeouts, use explicit waits for specific conditions, such as waiting for the input to have a certain attribute or value after clicking the apply button. Consider using expect with auto-retrying assertions or page.waitForFunction to wait for the state to update.
| await page.waitForTimeout(100); | |
| const currencyInput = page.locator('#currency-input'); | |
| // Wait for the currency input to be enabled and visible after applying settings | |
| const currencyInput = page.locator('#currency-input'); | |
| await expect(currencyInput).toBeVisible(); | |
| await expect(currencyInput).toBeEnabled(); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable inputValue.
| const inputValue = await currencyInput.inputValue(); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable inputValue.
| const inputValue = await currencyInput.inputValue(); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name "caret position is managed at end of input content" doesn't accurately describe what the test verifies. The test actually checks that text is selected when selectAllOnFocus is enabled. Consider renaming to something like "selects all text on focus when selectAllOnFocus is enabled" to better reflect the actual test behavior.
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't verify that selectAllOnFocus actually causes all text to be selected on focus. The test checks the checkbox and applies, then focuses the input, but only verifies that there's some selection (selectionEnd - selectionStart > 0). It doesn't verify that ALL text is selected by comparing the selection length to the total input value length. Consider checking that selectionStart === 0 and selectionEnd === inputValue.length to properly verify the selectAllOnFocus behavior.
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These input type tests duplicate functionality already covered in the new tests/input-type.spec.ts file. The dedicated input-type test file provides more comprehensive coverage of input type variations including tel, number, email, and formatting verification. Consider removing these tests from base.spec.ts to avoid duplication and reduce maintenance burden.
| test.describe('input type', () => { | |
| test('can set input type to email', async ({ page }) => { | |
| const inputTypeField = page.locator('[name=inputType]'); | |
| const applyBtn = page.locator('[name=apply]'); | |
| await inputTypeField.fill('email'); | |
| await applyBtn.click(); | |
| const currencyInput = page.locator('#currency-input'); | |
| const type = await currencyInput.getAttribute('type'); | |
| expect(type).toBe('email'); | |
| }); | |
| test('can set input type back to text', async ({ page }) => { | |
| const currencyInput = page.locator('#currency-input'); | |
| const type = await currencyInput.getAttribute('type'); | |
| expect(type).toBe('text'); | |
| }); | |
| }); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests verify that elements have the expected ID by selecting them with that ID and then checking the ID attribute. This is circular logic - if the locator '#currency-input' finds an element, that element by definition has id="currency-input". These tests don't add value and should be removed or replaced with tests that verify meaningful behavior or properties.
| test.describe('element attributes', () => { | |
| test('has correct id attribute', async ({ page }) => { | |
| const currencyInput = page.locator('#currency-input'); | |
| const id = await currencyInput.getAttribute('id'); | |
| expect(id).toBe('currency-input'); | |
| }); | |
| test('null-input-test element has correct id', async ({ page }) => { | |
| const nullInputTest = page.locator('#null-input-test'); | |
| const id = await nullInputTest.getAttribute('id'); | |
| expect(id).toBe('null-input-test'); | |
| }); | |
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import { test, expect } from '@playwright/test'; | ||
| import * as path from 'path'; | ||
|
|
||
| const projectDir = path.resolve(__dirname, '../'); | ||
| const filePath = path.join(projectDir, 'examples/index.html'); | ||
| const fileUrl = `file://${filePath}`; | ||
|
|
||
| test.describe('input type variations', () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.goto(fileUrl); | ||
| }); | ||
|
|
||
| test('default input type is text', async ({ page }) => { | ||
| const currencyInput = page.locator('#currency-input'); | ||
| await expect(currencyInput).toHaveAttribute('type', 'text'); | ||
| }); | ||
|
|
||
| test('set input type to tel for mobile keypad', async ({ page }) => { | ||
| const inputTypeField = page.locator('[name=inputType]'); | ||
| const applyBtn = page.locator('[name=apply]'); | ||
|
|
||
| await inputTypeField.fill('tel'); | ||
| await applyBtn.click(); | ||
|
|
||
| const currencyInput = page.locator('#currency-input'); | ||
| await expect(currencyInput).toHaveAttribute('type', 'tel'); | ||
|
|
||
| // Ensure formatting still works | ||
| await currencyInput.focus(); | ||
| await currencyInput.fill(''); | ||
| await currencyInput.type('123'); | ||
|
||
| await expect(currencyInput).toHaveValue('$1.23 USD'); | ||
| }); | ||
|
|
||
| test('set input type to number (native numeric)', async ({ page }) => { | ||
| const inputTypeField = page.locator('[name=inputType]'); | ||
| const applyBtn = page.locator('[name=apply]'); | ||
|
|
||
| await inputTypeField.fill('number'); | ||
| await applyBtn.click(); | ||
|
|
||
| const currencyInput = page.locator('#currency-input'); | ||
| await expect(currencyInput).toHaveAttribute('type', 'number'); | ||
|
|
||
| // Many browsers disallow arbitrary characters and formatting in native number inputs. | ||
| // Just verify that switching to number does not throw, then switch back to text. | ||
| await inputTypeField.fill('text'); | ||
| await applyBtn.click(); | ||
| await expect(currencyInput).toHaveAttribute('type', 'text'); | ||
| }); | ||
|
Comment on lines
+35
to
+50
|
||
|
|
||
| test('set input type to email (stress case) then back to text', async ({ page }) => { | ||
| const inputTypeField = page.locator('[name=inputType]'); | ||
| const applyBtn = page.locator('[name=apply]'); | ||
|
|
||
| await inputTypeField.fill('email'); | ||
| await applyBtn.click(); | ||
| await expect(page.locator('#currency-input')).toHaveAttribute('type', 'email'); | ||
|
|
||
| await inputTypeField.fill('text'); | ||
| await applyBtn.click(); | ||
| await expect(page.locator('#currency-input')).toHaveAttribute('type', 'text'); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent use of await with page.locator(). On line 31, currencyInput is declared with await, but locators are synchronous and don't need await. Compare with line 36 where the same pattern is used without await. Remove the await keyword for consistency with other locator declarations in the file.