🧪 (tests) adding custom report e2e tests #3493
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Smaller No assets were 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (2)
packages/desktop-client/e2e/page-models/reports-page.js (1)
23-28: LGTM: New goToCustomReportPage methodThe new
goToCustomReportPagemethod is well-implemented and consistent with the existing code style. It correctly navigates to the custom report creation page and returns a newCustomReportPageinstance.For consistency with other methods in this class, consider adding a comment describing the method's purpose.
Consider adding a comment above the method, similar to other methods in the class:
/** * Navigates to the custom report creation page. * @returns {Promise<CustomReportPage>} A new CustomReportPage instance. */ async goToCustomReportPage() { // ... (existing implementation) }packages/desktop-client/src/components/reports/GraphButton.tsx (1)
40-40: Consider handling undefined title for aria-labelWhile the current implementation is correct, there's an opportunity for a small improvement. Since the
titleprop is optional, you might want to consider providing a fallback value for thearia-labelwhentitleis undefined. This ensures that screen reader users always get meaningful information about the button's purpose.Here's a suggested modification:
- aria-label={title} + aria-label={title || 'Graph button'}This change provides a generic label when no specific title is available, enhancing the accessibility of the component in all scenarios.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (25)
packages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3493.mdis excluded by!**/*.md
Files selected for processing (4)
- packages/desktop-client/e2e/page-models/custom-report-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/reports-page.js (2 hunks)
- packages/desktop-client/e2e/reports.test.js (2 hunks)
- packages/desktop-client/src/components/reports/GraphButton.tsx (1 hunks)
Additional comments not posted (6)
packages/desktop-client/e2e/page-models/reports-page.js (2)
1-1: LGTM: Import statement for CustomReportPageThe import statement for
CustomReportPageis correctly added and necessary for the new functionality.
Line range hint
1-28: Summary: Effective addition of custom report navigationThe changes to
ReportsPagesuccessfully introduce the ability to navigate to the custom report creation page. This addition aligns well with the PR objectives of adding e2e tests for custom reports and addresses the technical debt mentioned in the linked issue.The new
goToCustomReportPagemethod is consistent with existing navigation methods and provides a clear way to interact with the custom report feature in e2e tests. These changes lay a good foundation for implementing the required e2e tests, including functional and visual regression testing for custom reports.packages/desktop-client/e2e/page-models/custom-report-page.js (2)
1-33: LGTM: Well-structured Page Object Model implementation.The
CustomReportPageclass follows the Page Object Model pattern, which is an excellent practice for e2e testing. It encapsulates the functionality required to interact with the custom report page, providing a clean and maintainable interface for tests.
1-33: Overall assessment: Well-implemented Page Object for custom report e2e tests.This new
CustomReportPageclass provides a solid foundation for e2e testing of custom reports, aligning well with the PR objectives. It follows good practices such as using the Page Object Model pattern and leveraging test IDs and roles for stable element selection.The suggestions provided earlier for minor improvements (adding methods for initialized buttons, error handling in
selectViz, and refactoringselectModefor more flexibility) would further enhance the robustness and maintainability of this class.Great job on implementing this crucial part of the e2e testing infrastructure for custom reports!
packages/desktop-client/src/components/reports/GraphButton.tsx (2)
40-40: Excellent accessibility improvement!The addition of the
aria-labelattribute to theButtoncomponent is a great enhancement for accessibility. This change ensures that screen readers can provide meaningful information about the button's purpose, which is especially important for graphical elements.This improvement aligns well with web accessibility best practices and contributes to making the application more inclusive. Great job on prioritizing accessibility!
40-40: Accessibility improvement supports testing objectivesAlthough this change doesn't directly add e2e tests, it enhances the quality of the component that will be tested. By improving accessibility, you're ensuring that the tests will be run on a component that follows best practices. This indirectly supports the PR objectives by:
- Potentially enabling more comprehensive accessibility testing in the future.
- Ensuring that the custom reports feature is built on a foundation of accessible components.
This proactive approach to improving component quality alongside adding tests is commendable and aligns well with the overall goal of enhancing the reliability and functionality of the custom reports feature.
MatissJanis
changed the title
* marked files for translation * added release note * fixed linting warnings * 🐛 fix account filters being overridden (#3441) *Reduce package size (#3443) * reduce package size of all packages * release notes * Update beforePackHook.ts * [Maintenance] Cleanup react aria packages and dedupe (#3450) * Cleanup react aria packages and dedupe * Release notes * ♻️ (synced-prefs) moving the prefs from metadata.json to the db (#3423) *
Closes #2998