adding more tests #45

ericblade · 2025-12-13T07:49:46Z

No description provided.

rollingversions · 2025-12-13T07:49:48Z

Change Log for @ericblade/react-currency-input (1.4.4 → 1.4.5)

Bug Fixes

fix not being able to set value, fix issue this fork is not compatible with original library since it uses uncontrolled input #44
fix that setting allowNegative on the fly wouldn't reformat negative sign out

Edit changelog

Change log has not been updated since latest commit Update Changelog

Copilot

Pull request overview

This PR adds comprehensive test coverage for a currency input component by creating a new test file and significantly expanding an existing one. The tests focus on component parameters, input type variations, formatting behaviors, and element attributes.

Creates a dedicated test suite for input type variations (text, tel, number, email)
Expands base tests with 280+ lines covering component parameters like prefix/suffix, separators, precision, and various configuration options
Tests basic input formatting, element attributes, and behavioral features like selectAllOnFocus

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 16 comments.

File	Description
tests/input-type.spec.ts	New file with 4 tests covering input type attribute changes (text, tel, number, email) and verifying formatting still works after type changes
tests/base.spec.ts	Expanded with 8 new test suites covering prefix/suffix, separators/precision, allowNegative, allowEmpty, selectAllOnFocus, input type, basic formatting, and element attributes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-13T07:53:26Z

tests/base.spec.ts

+            await decimalInput.fill(',');
+            await applyBtn.click();
+
+            await page.waitForTimeout(100);


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 · 2025-12-13T07:53:27Z

tests/base.spec.ts

+            await page.waitForTimeout(100);
+            const currencyInput = page.locator('#currency-input');


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.

Suggested change

await page.waitForTimeout(100);

const currencyInput = page.locator('#currency-input');

const currencyInput = page.locator('#currency-input');

await expect(currencyInput).toHaveValue('');

Copilot · 2025-12-13T07:53:27Z

tests/base.spec.ts

+            await selectAllCheckbox.check();
+            await applyBtn.click();
+
+            await page.waitForTimeout(100);


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.

Suggested change

await page.waitForTimeout(100);

tests/base.spec.ts

Copilot · 2025-12-13T07:53:29Z

tests/base.spec.ts

+    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');
+        });
+    });


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.

Suggested change

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');

});

});

Copilot · 2025-12-13T07:53:30Z

tests/base.spec.ts

+        test('renders with default prefix $ and suffix USD', async ({ page }) => {
+            const currencyInput = await page.locator('#currency-input');
+            await expect(currencyInput).toHaveValue('$0.00 USD');
+        });


This test duplicates the sanity check on line 13-16. Both tests verify that the currency input has the default value '$0.00 USD' without any additional setup or configuration. Consider removing this duplicate test.

Suggested change

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 · 2025-12-13T07:53:30Z

tests/input-type.spec.ts

+  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');
+  });


The test comment on line 45 states "Many browsers disallow arbitrary characters and formatting in native number inputs" but the test doesn't verify this behavior or demonstrate that the component handles it gracefully. The test immediately switches back to 'text' type without attempting any input with type='number'. Consider either adding assertions to verify behavior with number input type, or update the comment to accurately reflect what the test does (simply verifying that switching to and from 'number' type doesn't throw an error).

Copilot · 2025-12-13T07:53:30Z

tests/base.spec.ts

+
+    test.describe('separators and precision', () => {
+        test('default separators are comma thousand and period decimal', async ({ page }) => {
+            const currencyInput = page.locator('#currency-input');


Unused variable currencyInput.

Suggested change

const currencyInput = page.locator('#currency-input');

Copilot · 2025-12-13T07:53:30Z

tests/base.spec.ts

+
+            // With selectAllOnFocus, all text should be selected
+            // The selection should encompass the content
+            const inputValue = await currencyInput.inputValue();


Unused variable inputValue.

Suggested change

const inputValue = await currencyInput.inputValue();

- Modified getDerivedStateFromProps to distinguish between: - Value prop changes (parent controlling input) - always reformat - Display formatting changes (separators, prefix, suffix, precision) - reformat current value - Behavior changes (allowNegative, etc.) - preserve current input - This allows allowNegative to toggle without losing user's current input - Updated allowNegative tests to properly test toggling and negative number input - All 535 tests passing

- When allowNegative changes from true to false, any negative value is converted to positive - Added test to verify negative sign is removed when allowNegative is disabled - All 543 tests passing

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 16 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-13T22:36:22Z

tests/base.spec.ts

+            await page.waitForTimeout(100);
+            const currencyInput = page.locator('#currency-input');


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.

Suggested change

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 · 2025-12-13T22:36:23Z

src/index.tsx

-        const props = { ...nextProps };
-        if (nextProps.value !== prevState.value) {
-            props.value = prevState.value;
+        const previousProps = prevState.previousProps || nextProps; // First call has no previous props


The logic to initialize previousProps on first call is problematic. On the first call, prevState.previousProps will be undefined, so previousProps will be set to nextProps. This means on the very first render, the comparison on line 190 (nextProps.value !== previousProps.value) will always be false even if nextProps.value differs from the default or initial prop value. Consider initializing previousProps in the prepareProps method or handle the first call case explicitly.

Copilot · 2025-12-13T22:36:23Z

src/index.tsx

+        if (allowNegativeChanged && !nextProps.allowNegative) {
+            // allowNegative was disabled
+            // If current value is negative, make it positive
+            const currentValue = typeof prevState.value === 'number' ? prevState.value : 0;


The type check and handling for prevState.value is incomplete. The code checks if value is a number or defaults to 0, but prevState.value can also be a string according to the type definition. When it's a string, this code will default to 0, which may not reflect the actual value. Consider parsing string values properly using stringValueToFloat before checking if it's negative.

Suggested change

const currentValue = typeof prevState.value === 'number' ? prevState.value : 0;

let currentValue: number;

if (typeof prevState.value === 'number') {

currentValue = prevState.value;

} else if (typeof prevState.value === 'string') {

currentValue = stringValueToFloat(prevState.value, nextProps.decimalSeparator);

} else {

currentValue = 0;

}

Copilot · 2025-12-13T22:36:23Z

tests/base.spec.ts

+            await decimalInput.fill(',');
+            await applyBtn.click();
+
+            await page.waitForTimeout(100);


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.

Suggested change

await page.waitForTimeout(100);

await expect(decimalInput).toHaveValue(',');

Copilot · 2025-12-13T22:36:23Z

playwright.config.ts

    },
+    {
+      name: 'controlled-value tests',
+      testMatch: '**/controlled-value.spec.ts',


The new 'controlled-value tests' project doesn't specify any dependencies, unlike other test projects like 'mask tests' which depends on 'base tests'. If there are any setup requirements or if these tests should run after base tests complete, consider adding a dependencies array to ensure proper test execution order.

Suggested change

testMatch: '**/controlled-value.spec.ts',

testMatch: '**/controlled-value.spec.ts',

dependencies: ['base tests'],

Copilot · 2025-12-13T22:36:25Z

tests/base.spec.ts

+            await page.waitForTimeout(100);
+            const currencyInput = page.locator('#currency-input');


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.

Suggested change

await page.waitForTimeout(100);

const currencyInput = page.locator('#currency-input');

const currencyInput = page.locator('#currency-input');

await expect(currencyInput).toBeEnabled();

Copilot · 2025-12-13T22:36:25Z

tests/base.spec.ts

+            // Now disable allowNegative
+            await allowNegativeCheckbox.uncheck();
+            await applyBtn.click();
+            await page.waitForTimeout(200);


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.

Suggested change

await page.waitForTimeout(200);

await expect(currencyInput).not.toHaveValue(/-/);

Copilot · 2025-12-13T22:36:26Z

src/index.tsx

+            nextProps.thousandSeparator !== previousProps.thousandSeparator ||
+            nextProps.precision !== previousProps.precision ||
+            nextProps.prefix !== previousProps.prefix ||
+            nextProps.suffix !== previousProps.suffix;


The formatChanged check doesn't include inputType changes, but inputType can affect how the component behaves (as seen in line 169 where inputType === 'number' affects disableSelectionHandling). If inputType changes from 'text' to 'number' or vice versa, the component should update disableSelectionHandling in the state, but this isn't handled in the format change detection. Consider adding inputType to the formatChanged conditions or handling it separately.

Suggested change

nextProps.suffix !== previousProps.suffix;

nextProps.suffix !== previousProps.suffix ||

nextProps.inputType !== previousProps.inputType;

Copilot · 2025-12-13T22:36:26Z

tests/base.spec.ts

+
+    test.describe('separators and precision', () => {
+        test('default separators are comma thousand and period decimal', async ({ page }) => {
+            const currencyInput = page.locator('#currency-input');


Unused variable currencyInput.

Copilot · 2025-12-13T22:36:26Z

tests/base.spec.ts

+
+            // With selectAllOnFocus, all text should be selected
+            // The selection should encompass the content
+            const inputValue = await currencyInput.inputValue();


Unused variable inputValue.

Suggested change

const inputValue = await currencyInput.inputValue();

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-14T06:15:15Z

tests/input-type.spec.ts

+    // Ensure formatting still works
+    await currencyInput.focus();
+    await currencyInput.fill('');
+    await currencyInput.type('123');


Consider using pressSequentially() instead of type() for consistency with other test files and to ensure React events are properly triggered. The comment in tests/controlled-value.spec.ts:22 notes that pressSequentially is preferred to properly trigger React events.

tests/base.spec.ts

Copilot · 2025-12-14T06:15:16Z

src/index.tsx

+        if (formatChanged) {
+            // Display formatting changed - reformat the current value with new formatting
+            const propsWithCurrentValue = { ...nextProps, value: prevState.value };
+            const newState = CurrencyInput.prepareProps(propsWithCurrentValue);
+            return { ...newState, previousProps: nextProps };
+        }


The sequential if-statements mean that if multiple prop changes occur simultaneously, only the first matching condition is handled. For example, if both format props (like prefix) and allowNegative change at the same time, the formatChanged block executes and returns early, bypassing the special allowNegativeChanged logic that converts negative values to positive. Consider checking allowNegativeChanged before formatChanged, or include allowNegative in the formatChanged condition so that the special negative-to-positive conversion logic is always applied when allowNegative becomes false.

@copilot open a new pull request to apply changes based on this feedback

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot · 2025-12-14T06:17:05Z

@ericblade I've opened a new pull request, #46, to work on those changes. Once the pull request is ready, I'll request review from you.

… format prop changes (#46) * Initial plan * Fix getDerivedStateFromProps to handle simultaneous allowNegative and format prop changes Co-authored-by: ericblade <1451847+ericblade@users.noreply.github.com> * Add clarifying comment for valueToFormat initialization Co-authored-by: ericblade <1451847+ericblade@users.noreply.github.com> * Fix whitespace for consistency with codebase style Co-authored-by: ericblade <1451847+ericblade@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ericblade <1451847+ericblade@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/base.spec.ts

tests/controlled-value.spec.ts

adding more tests

18a6faf

Copilot AI review requested due to automatic review settings December 13, 2025 07:49

Copilot started reviewing on behalf of ericblade December 13, 2025 07:50 View session

Copilot AI reviewed Dec 13, 2025

View reviewed changes

ericblade added 3 commits December 13, 2025 04:21

fix #44, setting value should work

ecb5a9a

Make negative numbers positive when allowNegative is disabled

2699b8f

- When allowNegative changes from true to false, any negative value is converted to positive - Added test to verify negative sign is removed when allowNegative is disabled - All 543 tests passing

ericblade requested a review from Copilot December 13, 2025 22:10

Copilot started reviewing on behalf of ericblade December 13, 2025 22:10 View session

Copilot AI reviewed Dec 13, 2025

View reviewed changes

some more adjustment of allowNegativeChanged

16c2a2c

ericblade requested a review from Copilot December 14, 2025 06:09

Copilot started reviewing on behalf of ericblade December 14, 2025 06:09 View session

Copilot AI reviewed Dec 14, 2025

View reviewed changes

Update tests/base.spec.ts

701ebcf

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI mentioned this pull request Dec 14, 2025

Fix getDerivedStateFromProps to handle simultaneous allowNegative and format prop changes #46

Merged

ericblade requested a review from Copilot December 16, 2025 03:29

Copilot started reviewing on behalf of ericblade December 16, 2025 03:29 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

tests/base.spec.ts Show resolved Hide resolved

tests/base.spec.ts Show resolved Hide resolved

tests/base.spec.ts Show resolved Hide resolved

tests/base.spec.ts Show resolved Hide resolved

tests/controlled-value.spec.ts Show resolved Hide resolved

ericblade merged commit 7fdddbb into master Dec 16, 2025
7 of 8 checks passed

ericblade deleted the dev branch December 16, 2025 06:03

		await page.waitForTimeout(100);
		const currencyInput = page.locator('#currency-input');

-            const currentValue = typeof prevState.value === 'number' ? prevState.value : 0;
+            let currentValue: number;
+            if (typeof prevState.value === 'number') {
+                currentValue = prevState.value;
+            } else if (typeof prevState.value === 'string') {
+                currentValue = stringValueToFloat(prevState.value, nextProps.decimalSeparator);
+            } else {
+                currentValue = 0;
+            }

	await page.waitForTimeout(100);
	await expect(decimalInput).toHaveValue(',');

	testMatch: '**/controlled-value.spec.ts',
	testMatch: '**/controlled-value.spec.ts',
	dependencies: ['base tests'],

	await page.waitForTimeout(200);
	await expect(currencyInput).not.toHaveValue(/-/);

	nextProps.suffix !== previousProps.suffix;
	nextProps.suffix !== previousProps.suffix \|\|
	nextProps.inputType !== previousProps.inputType;

adding more tests #45

adding more tests #45

Uh oh!

Conversation

ericblade commented Dec 13, 2025

Uh oh!

rollingversions bot commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Log for @ericblade/react-currency-input (1.4.4 → 1.4.5)

Bug Fixes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

rollingversions bot commented Dec 13, 2025 •

edited

Loading