test: csv import e2e tests #3499
Conversation
there was an issue with css precedent which I don't understand why but this fixes this
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces a new test suite for importing transactions from CSV files, modifies the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
UnderKoen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
packages/desktop-client/e2e/page-models/close-account-modal.js (1)
Line range hint
1-18: Consider documenting the usage ofcloseAccountvsforceCloseAccountThe addition of the
forceCloseAccountmethod provides more flexibility in account closure scenarios, which is good. However, to prevent potential misuse or confusion, consider the following:
- Add class-level documentation explaining the difference between
closeAccountandforceCloseAccount, and when each should be used.- Ensure that the main application has appropriate safeguards when using the "force close" functionality, if it corresponds to bypassing normal closure checks.
Here's a suggested addition to the class documentation:
/** * Represents the Close Account modal in the application. * This class provides methods to interact with the modal during end-to-end testing. * * The class offers two methods for closing an account: * - closeAccount(): For standard account closure. This should be the primary method used. * - forceCloseAccount(): For scenarios where standard closure fails or specific test cases. * Use with caution as it may bypass normal closure checks. */ export class CloseAccountModal { // ... existing code ... }This documentation clarifies the purpose of each method and provides guidance on their usage.
packages/desktop-client/e2e/accounts.test.js (1)
120-132: Good encapsulation, consider parameterizing the file path.The
importCsvfunction effectively encapsulates the CSV import logic, making it reusable across tests. The use ofpage.waitForEvent('filechooser')is a good practice for handling file inputs in Playwright.Consider parameterizing the CSV file path instead of hard-coding it:
-async function importCsv(screenshot = false) { +async function importCsv(csvFilePath, screenshot = false) { const fileChooserPromise = page.waitForEvent('filechooser'); await accountPage.page.getByRole('button', { name: 'Import' }).click(); const fileChooser = await fileChooserPromise; - await fileChooser.setFiles(join(__dirname, 'data/test.csv')); + await fileChooser.setFiles(csvFilePath); if (screenshot) await expect(page).toMatchThemeScreenshots(); await accountPage.page .getByRole('button', { name: /Import \d+ transactions/ }) .click(); }This change would make the function more flexible and allow testing with different CSV files if needed.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (8)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/data/test.csvis excluded by!**/*.csvupcoming-release-notes/3499.mdis excluded by!**/*.md
Files selected for processing (4)
- packages/desktop-client/e2e/accounts.test.js (2 hunks)
- packages/desktop-client/e2e/page-models/close-account-modal.js (1 hunks)
- packages/desktop-client/src/components/modals/CloseAccountModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal.jsx (1 hunks)
Additional comments not posted (9)
packages/desktop-client/e2e/page-models/close-account-modal.js (1)
Line range hint
1-18: Overall assessment of changesThe addition of the
forceCloseAccountmethod enhances the functionality of theCloseAccountModalclass without disrupting existing code. This change appears to be in line with the PR objectives of improving e2e tests for account-related operations.To further improve the implementation:
- Enhance error handling and return values in the new method.
- Add comprehensive documentation to clarify the usage of different account closure methods.
- Ensure that the main application handles "force close" operations safely.
These improvements will contribute to more robust and maintainable e2e tests.
packages/desktop-client/e2e/accounts.test.js (3)
1-1: LGTM: Appropriate import for file path handling.The addition of the
joinfunction from the 'path' module is appropriate for handling file paths in the new CSV import tests.
Line range hint
1-143: Overall good addition of CSV import tests, with room for improvements.The new test suite for CSV import functionality is a valuable addition to the e2e tests. The structure and approach are good, with proper setup and teardown procedures. However, there are several areas where the tests can be improved:
- Consider parameterizing the CSV file path in the
importCsvfunction for better flexibility.- Add assertions to both test cases to verify the success and correctness of the imports.
- Replace the fixed wait time in the second test with a more reliable wait condition.
- Clarify the usage of
forceCloseAccount()in the teardown procedure.Addressing these points will enhance the reliability and effectiveness of the CSV import tests.
105-118: Good test isolation, but clarify force close usage.The setup and teardown procedures ensure a clean state for each test, which is a good practice. However, the use of
forceCloseAccount()in the teardown might need explanation.Could you provide more context on why
forceCloseAccount()is necessary? Is there a specific reason for not using the regular close method?Verification successful
Good test isolation; force close method is appropriate.
The setup and teardown procedures ensure a clean state for each test. The use of
forceCloseAccount()in the teardown appropriately ensures that the account is closed without dependencies.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of forceCloseAccount rg "forceCloseAccount" --type jsLength of output: 209
Script:
#!/bin/bash # Retrieve the implementation of forceCloseAccount from close-account-modal.js rg "async forceCloseAccount" --context 5 packages/desktop-client/e2e/page-models/close-account-modal.jsLength of output: 303
packages/desktop-client/src/components/modals/ImportTransactionsModal.jsx (5)
744-749: Improved spacing for Date field.The
marginRightstyle has been moved from theSelectFieldto the parentViewcomponent, and increased from 5 to 10 pixels. This change provides more consistent spacing between fields.
754-759: Improved spacing for Payee field.Similar to the Date field, the
marginRightstyle has been moved to the parentViewcomponent and increased to 10 pixels. This change maintains consistency with the other fields.
764-769: Improved spacing for Notes field.The same spacing improvement has been applied to the Notes field, ensuring a uniform layout across all fields in the
FieldMappingscomponent.
774-779: Improved spacing for Category field.The Category field has also received the same spacing adjustment, completing the uniform layout for all main fields in the
FieldMappingscomponent.
744-779: Overall improvement in form layout and spacing.The changes made to the
FieldMappingscomponent consistently apply amarginRightof 10 pixels to the parentViewcomponents of each field (Date, Payee, Notes, and Category). This modification results in:
- Improved visual consistency across all fields.
- Better spacing between fields, enhancing readability.
- A more polished and professional appearance for the form.
These changes are purely cosmetic and do not affect the functionality of the component. They represent a positive improvement in the user interface design.
packages/desktop-client/src/components/modals/CloseAccountModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
👋 I ran the tests 5x to check for flakiness. There is a bit of flakiness (2 failures out of 7x runs). Would you mind checking them to see if something could be improved?
Here's a sample flake: https://github.com/actualbudget/actual/actions/runs/11040670144/job/30669239338?pr=3499
(ignore the other tests also flaking - I'm working on getting them in top shape too)
packages/desktop-client/src/components/modals/CloseAccountModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/e2e/accounts.test.js (2)
114-129: Approve helper function with a minor suggestionThe
importCsvhelper function is well-implemented and encapsulates the CSV import process effectively. It's a good practice to usepage.waitForEvent('filechooser')for handling file uploads in Playwright tests.However, there's a minor improvement to be made:
Change the
letdeclaration ofimportButtontoconstas it's never reassigned:- let importButton = accountPage.page.getByRole('button', { + const importButton = accountPage.page.getByRole('button', {This will address the lint warning and improve code quality.
🧰 Tools
🪛 GitHub Check: lint
[warning] 123-123:
'importButton' is never reassigned. Use 'const' instead
135-138: Approve double import test with a suggestionThis test case for importing the same CSV file twice is valuable. It checks whether the CSV options get greyed out when transactions are matched with existing ones, which is an important behavior to verify.
To improve clarity, consider adding a comment explaining the purpose of this test:
test('import csv file twice', async () => { // First import to populate the account with transactions await importCsv(false); // Second import to verify that CSV options are greyed out for matching transactions await importCsv(true); });This comment will help future maintainers understand the test's intention more quickly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (12)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-6-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
- packages/desktop-client/e2e/accounts.test.js (4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/e2e/accounts.test.js (1)
Learnt from: UnderKoen PR: actualbudget/actual#3499 File: packages/desktop-client/e2e/accounts.test.js:134-136 Timestamp: 2024-09-24T20:19:41.500Z Learning: In the 'accounts.test.js' test suite, when testing CSV import functionality, the test results are verified visually through screenshots rather than with explicit assertions.
🪛 GitHub Check: lint
packages/desktop-client/e2e/accounts.test.js
[warning] 123-123:
'importButton' is never reassigned. Use 'const' instead
🔇 Additional comments (3)
packages/desktop-client/e2e/accounts.test.js (3)
Line range hint
14-23: Improved test isolation withbeforeEachandafterEachThe change from
beforeAllandafterAlltobeforeEachandafterEachis a good improvement. This ensures that each test runs in a fresh environment, reducing the risk of test interdependencies and improving overall test reliability.
105-112: Well-structured test suite for CSV importThe new "Import Transactions" test suite is well-organized. Creating a fresh "CSV import" account before each test ensures proper isolation and a consistent starting point for import tests.
131-133: Approve concise CSV import testThis test case for importing transactions from a CSV file is concise and focused. It leverages the
importCsvhelper function effectively.As per the project's convention, the test results are verified visually through screenshots rather than explicit assertions. This approach is consistent with other tests in the suite.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/e2e/accounts.test.js (2)
114-129: Well-implementedimportCsvhelper functionThe
importCsvhelper function is well-implemented and encapsulates the CSV import process effectively. The use ofwaitForEventfor the file chooser is a good practice for handling asynchronous events.Consider adding a wait condition after clicking the import button to ensure the import process has completed before the function returns. This could improve test reliability, especially for larger CSV files.
Here's a suggested improvement:
async function importCsv(screenshot = false) { // ... existing code ... await importButton.click(); // Wait for import to complete await page.waitForSelector('.import-complete-indicator', { state: 'visible' }); if (screenshot) await expect(page).toMatchThemeScreenshots(); }Replace
.import-complete-indicatorwith an appropriate selector that indicates the import has completed.
135-138: Effective test for double CSV importThis test case effectively verifies the behavior when importing the same CSV file twice. The visual verification approach is appropriate for checking if CSV options are greyed out on the second import, as discussed in previous comments.
To improve clarity, consider adding a comment explaining the purpose of the second import and what the screenshot is expected to show.
Here's a suggested improvement:
test('import csv file twice', async () => { await importCsv(false); // Second import should show greyed out CSV options for matched transactions await importCsv(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/e2e/accounts.test.js (4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/e2e/accounts.test.js (1)
Learnt from: UnderKoen PR: actualbudget/actual#3499 File: packages/desktop-client/e2e/accounts.test.js:134-136 Timestamp: 2024-09-24T20:19:41.500Z Learning: In the 'accounts.test.js' test suite, when testing CSV import functionality, the test results are verified visually through screenshots rather than with explicit assertions.
🔇 Additional comments (4)
packages/desktop-client/e2e/accounts.test.js (4)
Line range hint
14-23: Improved test isolation withbeforeEachandafterEachThe change from
beforeAllandafterAlltobeforeEachandafterEachis a good improvement. This ensures that each test runs in a fresh environment, reducing the risk of inter-test dependencies and improving overall test reliability.
105-112: Well-structured test suite for CSV importThe new "Import Transactions" test suite is well-organized. The
beforeEachhook that creates a fresh "CSV import" account for each test ensures consistent starting conditions, which is a good practice for reliable testing.
131-133: Simple and focused CSV import testThis test case for importing transactions from a CSV file is straightforward and focused. As per the provided learning, the test results are verified visually through screenshots rather than with explicit assertions, which is an acceptable approach for this scenario.
Line range hint
1-139: Overall improvement in CSV import testingThe changes introduced in this file significantly enhance the test coverage for CSV import functionality. The new test suite is well-structured, with a dedicated helper function and two focused test cases. The use of visual verification through screenshots is appropriate for this scenario, as it allows for checking complex UI states that might be difficult to assert programmatically.
The shift to
beforeEachandafterEachhooks improves test isolation, which is a good practice for maintaining reliable tests. While this approach might slightly increase test execution time, it provides better consistency and reduces the risk of inter-test dependencies.These changes align well with the PR objectives of reintroducing CSV import e2e tests and addressing related issues. The implementation appears complete and ready for review.
There was a problem hiding this comment.
Re-ran a couple of times and the tests are now stable! Thanks!
Would you mind fixing the conflict? I must have just introduced it when merging #3552
# Conflicts: # packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.png # packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.png # packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.png # packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-4-chromium-linux.png # packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx
UnderKoen
force-pushed
the
fix/csv-import-3416
branch
from
c62946d to
e2e9903
Compare
There was a problem hiding this comment.
Is there a reason these would have changed? Looks like we're missing "New Account" from the sidebar.
Same in all Accounts-closes screenshots
There was a problem hiding this comment.
It makes an new test file for each test now. It was unstable before, run order wasn't always the same, especially when the first try failed
Readds #3467
The problem that caused #3474 was something to do with css precedent