Add Comprehensive Appium E2E Testing Framework for Sefaria Mobile App#175
Add Comprehensive Appium E2E Testing Framework for Sefaria Mobile App#175
Conversation
… Removed .only from testing so all tests can run
|
@akiva10b @yitzhakc @schreibersefaria Pull Request created. Woohoo Automatic tests! |
…t, and improved swipe functionality
…ions, and creating texts tab check (almost done)
…gewt the topics page and specific topics, enhanced ui checker for specific screenshot checking, and continued to use newly implemented text_constants
schreibersefaria
left a comment
schreibersefaria
left a comment
There was a problem hiding this comment.
It would be helpful if you fix up the readme before I continue the PR.
Alternatively, could you add to the PR description more details about each file and the general structure.
automatic-e2e-tests/android/testing-framework/components/topics_page.ts
Outdated
Show resolved
Hide resolved
Seperated the README into 4 seperate components for better instructions and clarity. (SETUP, TEST_GUIDE, FILE_OVERVIEW, README). This creates greater understanding of what everything does, how to create tests, and easier way to run them. Also, added headers to each file, explaining what each file does, its purpose, and its usage.
…f concerns. FILE_OVERVIEW reflects this change
schreibersefaria
left a comment
schreibersefaria
left a comment
There was a problem hiding this comment.
I'm still (almost) only commenting on the MD files but enough has accumulated to push the review.
I think it would make sense to huddle to discuss the PR before you put too much work into it and before I continue to the rest of the content
…xed browserstack error where it would not launch app.
…ainability - Introduced centralized constants for gestures, colors, timeouts, and selectors to replace hardcoded values across the testing framework. - Updated gesture utility functions to utilize new gesture configuration constants for swipe distances and durations. - Refactored UI interaction utilities to use centralized selectors for offline popups and text finding, enhancing consistency. - Enhanced timeout management by consolidating timeout values into a dedicated constants file for better readability and maintenance. - Improved documentation across various utility files to clarify usage and structure.
- Consolidated static and dynamic error messages into structured objects in error_constants.ts for better organization and maintainability. - Updated all references to error messages throughout the codebase to use the new structure. - Removed unused gesture validation thresholds from gestures.ts. - Cleaned up selectors.ts by adding a new selector for content description. - Removed unnecessary timeout constants from timeouts.ts. - Enhanced logging in e2e tests to provide clearer information on test execution status. - Updated helper functions and UI checker utilities to utilize the new error handling structure. - Improved debug logging for better traceability during test execution.
… and maintainability across multiple components
… consistency. Removed repeated code.
… and update related test cases. All tests pass
schreibersefaria
left a comment
schreibersefaria
left a comment
There was a problem hiding this comment.
Hope to do some more tomorrow
| Central import point for all components, allowing easy access to all page objects. | ||
|
|
||
| - **\*.ts** | ||
| Add new component helpers as needed. |
There was a problem hiding this comment.
ahh I think it's understood implicitly
| - Upload your APK/AAB (Android) or IPA (iOS) file to BrowserStack using their [App Upload page](https://app-automate.browserstack.com/dashboard/v2/app-upload). | ||
| - Copy the App ID (e.g., `bs://<app-id>`) and set it in your `.env` as `ANDROID_BROWSERSTACK_APP_ID` or `IOS_BROWSERSTACK_APP_ID`: | ||
| ```env | ||
| - **Note:** You must upload your app every time you build a new APK/AAB/IPA. The App ID changes with each upload, so always update `*_BROWSERSTACK_APP_ID` in your `.env`. |
There was a problem hiding this comment.
mmm in that case personally I would prefer not to have it at all.
I'd probable just run it locally anyway. I'm not sure I even have permissions to edit the secrets
| if (parasha) { | ||
| await TEXT_FINDER.findTextElement(client, parasha.displayValue.en); | ||
| } else { | ||
| throw new Error(DYNAMIC_ERRORS.apiResultMismatch("Parashat Hashavua", parasha!.displayValue.en)); |
There was a problem hiding this comment.
but this if is on SEFARIA_API.getCurrentParashatHashavua not TEXT_FINDER
There was a problem hiding this comment.
Not sure where the conversation about this is so opening it again.
I think it makes more sense to have the tests in logical files.
You would then define what tests are regression somewhere else.
There was a problem hiding this comment.
Hmmm, logically that makes sense. I will have to go through and possible edit my parallel running script if I do that.
schreibersefaria
left a comment
schreibersefaria
left a comment
There was a problem hiding this comment.
I've added some comments that generalise to most the code.
The big things I noticed are error handling and return values.
In general, consistency is key (but very hard to do).
| * @param client - WebdriverIO browser instance. | ||
| * @param isEnglish - Current language state; true if English, false if Hebrew. | ||
| */ | ||
| export async function toggleLanguageButton(client: Browser, isEnglish: boolean = true): Promise<void> { |
There was a problem hiding this comment.
I don't think you need isEnglish, you can just check the current language.
It could be more elegant to have a param of targetLanguage, then you check if we are in the target language and only change if needed.
…o enhance clarity and maintainability
…larity. Turn Uppercase into PascalCase, and working on continuing to make imports better and more readable
…dication and improve error handling with helper function HelperFunctions.ensureElementDisplayed().
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive end-to-end testing framework for the Sefaria mobile app using WebdriverIO and Appium, supporting both Android and iOS platforms with local and cloud testing capabilities.
- Modular, cross-platform architecture with centralized constants and platform-specific selectors
- Automated UI validation including color pixel testing and visual regression capabilities
- Integration with Sefaria APIs for dynamic content validation and BrowserStack for cloud testing
Reviewed Changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/ui_checker.ts | Pixel color validation utilities with cross-platform coordinate system handling |
| utils/text_finder.ts | Text and element location helpers for robust UI interaction |
| utils/sefaria_api.ts | API utilities for fetching calendar data and learning schedules |
| utils/offline_popup.ts | Popup handling utilities for app initialization flows |
| utils/load_credentials.ts | Platform-agnostic credential loader for local and BrowserStack configurations |
| utils/helper_functions.ts | Core helper functions for text matching, color conversion, and test lifecycle |
| utils/gesture.ts | Gesture and scrolling utilities with platform-aware element visibility |
| utils/browserstack_report.ts | BrowserStack session reporting and status management |
| constants/*.ts | Centralized configuration for timeouts, colors, gestures, and error messages |
| components/*.ts | Page object models for navbar, reader, search, topics, and display settings |
| selectors/*.ts | Platform-specific UI selectors for Android and iOS |
| package.json | Dependencies and test execution scripts |
| Other files | Configuration, documentation, and test execution infrastructure |
Comments suppressed due to low confidence (1)
automatic-e2e-tests/constants/errors.ts:1
- Using non-null assertion
parasha!.displayValue.enwhenparashawas already checked to be falsy in the condition above. This will throw a runtime error. The error message should use a default value or handle the undefined case properly.
/**
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…ance error logging in ensureElementDisplayed function
…ces in documentation and code
…Added ability to close pop ups (closing it whenever it pops up in random intervals is still work in progress)
…elper functions with screenshot capture on failure. Working on speed and closing popups.
…one session instead of multiple versions.
…screenshot functionality. (Improving testing time)
… the use case of the file structure.
…top checking for specific types of popUps that appear.
… This is for setup and reporting to browserstack to make it mkore efficient.
…ne. Increases speed.
schreibersefaria
left a comment
schreibersefaria
left a comment
There was a problem hiding this comment.
A few big comments and some smaller ones.
Overall it looks to be going in the right direction but there are some big changes still ahead of us.
As you may notice I haven't yet gone over every line so after you get to all the comments, please request another review and ping me.
And you don't have to wait for the next review for questions. I'm generally available :-)
| Centralized logs and screenshots for both platforms. | ||
|
|
||
| - **scripts/** | ||
| Utility scripts (e.g., cleanup.js). `run-parallel-tests.js` for running tests in parallel on multiple devices. |
There was a problem hiding this comment.
You give two examples. But one is in brackets and one isn't.
I'd would unify the format here
|
|
||
| --- | ||
|
|
||
| ## components/ |
There was a problem hiding this comment.
Confusing styling.
Some dirs are h2 and some are bold.
Also, I'd order the dirs by importance
There was a problem hiding this comment.
I'm not sure why this is needed.
I understood the logic around selectors but not sure it duplicates to other dirs.
| Selectors = require('../selectors/android/selectors'); | ||
| } | ||
|
|
||
| export { Selectors }; |
There was a problem hiding this comment.
I find it confusing that you combined the index of constants and selectors.
Was there a reason for that
There was a problem hiding this comment.
Here too, if there isn't a unified ios/android import, I don't know if there is a need for the index file.
If people need to import a lot of constance then a long import line / many imports are appropriate
| export const APP_PACKAGE = 'org.sefaria.sefaria'; | ||
|
|
||
| // Base UiSelector patterns | ||
| export const BASE_SELECTORS = { |
There was a problem hiding this comment.
Another naming comment.
Not sure why selectors would be all caps. They are functions.
This could be a best practice I'm not familiar with
|
|
||
| // iOS Accessibility patterns | ||
| export const ACCESSIBILITY_PATTERNS = { | ||
| englishTextPrefix: '', |
There was a problem hiding this comment.
If you are using a hidden unicode char you should definitely comment it
There was a problem hiding this comment.
Generally in both selector files, some more commenting would be very helpful for anyone who needs to edit it
| * @throws Will throw an error if the text is not found | ||
| * @returns boolean indicating success | ||
| */ | ||
| export async function verifyTitleContains(client: Browser, expectedText: string): Promise<boolean> { |
There was a problem hiding this comment.
A lot of duplication from the previous function.
Could you combine them?
There was a problem hiding this comment.
You have one document for all topic related things.
Maybe split each page within topics (the actual topic page and the navigation pages) into different files
4 participants
This PR introduces a robust, modular end-to-end (E2E) testing framework for the Sefaria mobile app, built with WebdriverIO and Appium. The framework enables automated testing on Android devices both locally and via BrowserStack, supporting continuous integration with GitHub Actions.
Key Features:
Modular test structure for easy maintenance and expansion
Automated logging of test runs and UI errors (with screenshots)
Support for local and cloud (BrowserStack) device testing
Clear documentation for setup, running, and contributing new tests
Utilities for interacting with app UI, navigation, and Sefaria APIs
Example tests covering navigation, language toggling, learning schedules, and more
Why this matters:
Automated E2E testing will help ensure the quality and reliability of the Sefaria app across updates, reduce manual QA effort, and make it easier for contributors to add new tests as features evolve.
How to get started:
See the included README.md for setup instructions, prerequisites, and contribution guidelines. All contributors are welcome to add new tests or improve existing ones!