Add pytest-bdd + Appium App Automate sample (Android + iOS)#1
Add pytest-bdd + Appium App Automate sample (Android + iOS)#1AakashHotchandani wants to merge 9 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…t-bound bs://) — parity with nunit-appium sibling
…dd past .gitignore)
shivam5643
left a comment
shivam5643
left a comment
There was a problem hiding this comment.
Review: pytest-bdd + Appium App Automate sample
Verdict: Request changes. The structure, READMEs, and the Wikipedia/iOS-text happy paths look solid, but the BrowserStack Local scenarios are non-functional as configured, and the README/CI/config contradict each other in ways that will burn customers who copy this repo.
🔴 Blocking issues
1. browserstackLocal: true is documented everywhere but present in neither browserstack.yml.
README.mdand bothtests/test_local.pydocstrings state the local tunnel auto-starts "withbrowserstackLocal: trueinbrowserstack.yml."- But
android/browserstack.ymlandios/browserstack.ymldo not contain that key (both end atnetworkLogs: true). browserstack-localis inrequirements.txt, but without the config flag the SDK won't start the tunnel. The local scenarios cannot work as shipped.
2. The Local sample apps are committed but never wired into any config — the local scenarios install the wrong app and will fail.
- App Automate installs a single
app:per run. Both yml files pinapp: ./WikipediaSample.apk(android) /./BStackSampleApp.ipa(ios). LocalSample.apk/LocalSample.ipaare committed but referenced nowhere inbrowserstack.yml.- So when
browserstack-sdk pytest -s tests/runs both scenarios in one build,test_localexecutes against the Wikipedia/BStack app, where its selectors don't exist:- android:
AppiumBy.ID, "com.example.android.basicnetworking:id/test_action"→NoSuchElement - ios:
AppiumBy.ACCESSIBILITY_ID, "TestBrowserStackLocal"→NoSuchElement
- android:
- Yet
README.mdexplicitly claims "It also runs the local scenario (LocalSample.apk over the BrowserStack Local tunnel)." That's not achievable with oneapp:value. You'd need a separate config/build (ormidSessionInstallApps) for the local app. Either remove the local scenarios or give them their own directory/config and document the two-build flow.
3. README contradicts the actual app: config (and the PR body / CI note).
- README Prerequisites: "The Android directory is pre-wired to a pre-uploaded
WikipediaSample.apk(bs://...)" and the CI note in.github/workflows/sdk-sample-test.ymltalks aboutapp: bs://...apps expiring. - Actual config is
app: ./WikipediaSample.apk(local path, uploaded at runtime) — the opposite of "pre-uploadedbs://." - The android yml comment is also garbled/self-contradictory:
First line is truncated and says the opposite of the second. Pick one model (local-path upload is the simpler, account-agnostic choice for a public sample) and make README + both yml comments + CI note agree.
# Pre-uploaded WikipediaSample.apk. Use a local path (./WikipediaSample.apk) to # Public sample app committed in this repo (relative path); the SDK uploads it at run time.
🟠 Should fix
4. PR claims "live-validated" but CI only exercises one of four paths. sdk-sample-test.yml sets working-directory: android and runs only tests/test_sample.py — never the local scenario, never iOS. Given #1/#2, the local + iOS paths almost certainly were not green. Either extend CI to a matrix over {android, ios} × {sample, local} (with the local build using its own app config) or soften the "live-validated" claim. Right now CI silently passes while two-thirds of the repo is untested.
5. conftest.py contradicts its own docstring and duplicates creds. Both fixtures hardcode webdriver.Remote("https://hub.browserstack.com/wd/hub", ...) and stuff userName/accessKey into bstack:options, while the docstring says "no hub URL caps … are set here" and credentials already live in browserstack.yml. Under browserstack-sdk the hub/caps are SDK-managed; the manual hub URL + cred duplication is redundant and is the kind of thing a customer will copy into a real project and then wonder why env-var creds aren't honored. Recommend a bare UiAutomator2Options() / XCUITestOptions() and let the SDK inject everything (which is exactly what the docstring promises).
🟡 Minor / polish
- android
test_sample.pyassertion is a no-op:assert len(results) > 0on allTextViews is true the moment the app opens, before any search. It won't catch a broken search. Assert on result rows / a known result text instead. time.sleep(5)in the android search step — fine for a sample, but aWebDriverWaitreads better as exemplary customer code.- Pinning mismatch:
pytest==7.4.4hard-pinned whilepytest-bddis unpinned — a future pytest-bdd release can break the pinned pytest. Pin both or neither.
✅ What's good
- Clean two-directory
android//ios/split, each self-contained — easy for a customer to copy one platform. - Gherkin features + step defs are readable and idiomatic pytest-bdd.
source:tagging andtestObservability: trueare correct for a sample.
Bottom line: the sample happy-paths (Wikipedia search / iOS text echo) are good, but ship-blockers #1–#3 mean the Local feature is broken and the docs misdescribe the app-provisioning model. I'd fix the Local config (or pull those scenarios), reconcile README/CI/yml on the app: model, and widen CI before merge.
🤖 Generated with Claude Code
shivam5643
left a comment
shivam5643
left a comment
There was a problem hiding this comment.
Inline comments for the findings in my review above — anchored to the specific lines.
| # Debugging features | ||
| # =================== | ||
| debug: true | ||
| networkLogs: true |
There was a problem hiding this comment.
🔴 Blocking #1: browserstackLocal: true is missing here. The README and both tests/test_local.py docstrings claim the tunnel auto-starts "with browserstackLocal: true in browserstack.yml", and browserstack-local is in requirements.txt — but without this key the SDK never starts the tunnel, so the local scenario can't work. Add browserstackLocal: true.
| # Debugging features | ||
| # =================== | ||
| debug: true | ||
| networkLogs: true |
There was a problem hiding this comment.
🔴 Blocking #1 (iOS): same as android — browserstackLocal: true is missing, so the local-tunnel scenario can't function despite the docs/requirements implying it does.
| # ========================================== | ||
| # Application under test | ||
| # ========================================== | ||
| # Pre-uploaded WikipediaSample.apk. Use a local path (./WikipediaSample.apk) to |
There was a problem hiding this comment.
🔴 Blocking #3: this comment is garbled and self-contradictory — line 1 ("Pre-uploaded WikipediaSample.apk. Use a local path … to") is truncated and says the opposite of line 2. It also contradicts the README Prerequisites ("pre-wired to a pre-uploaded WikipediaSample.apk (bs://...)") and the CI bs:// note, while the actual config below uses a local path. Pick one model (local-path upload is the simpler choice for a public sample) and make README + both yml comments + CI agree.
| @when("I trigger the local network test") | ||
| def trigger_local_test(driver): | ||
| driver.find_element( | ||
| AppiumBy.ID, "com.example.android.basicnetworking:id/test_action").click() |
There was a problem hiding this comment.
🔴 Blocking #2: App Automate installs a single app: per run, and android/browserstack.yml pins app: ./WikipediaSample.apk. LocalSample.apk is committed but referenced nowhere, so this step runs against the Wikipedia app where com.example.android.basicnetworking:id/test_action doesn't exist → NoSuchElement. The README's claim that pytest -s tests/ "also runs the local scenario (LocalSample.apk)" isn't achievable with one app: value. Give the local scenario its own directory/config (or use midSessionInstallApps) and document the two-build flow — or drop it.
| @when("I trigger the local network test") | ||
| def trigger_local_test(driver): | ||
| driver.find_element(AppiumBy.ACCESSIBILITY_ID, "TestBrowserStackLocal").click() | ||
|
|
There was a problem hiding this comment.
🔴 Blocking #2 (iOS): same problem — ios/browserstack.yml installs BStackSampleApp.ipa, not LocalSample.ipa, so TestBrowserStackLocal won't be found. The local app is committed but never wired into any config.
| pip install -r requirements.txt | ||
| - name: Run sample test | ||
| run: | | ||
| browserstack-sdk pytest tests/test_sample.py |
There was a problem hiding this comment.
🟠 Should fix #4: combined with working-directory: android above, CI only ever runs the android sample test — never the local scenario, never iOS. Given blocking #1/#2, the local + iOS paths were almost certainly not green. The PR body says "live-validated"; either extend CI to a {android, ios} × {sample, local} matrix (local using its own app config) or soften that claim.
| # Runs the BrowserStack SDK sample against a given commit and reports a status check. | ||
| # Trigger: Actions tab -> "pytest-bdd Appium App Automate SDK sample test" -> Run workflow -> paste the PR's full commit SHA. | ||
| # Requires repo secrets: BROWSERSTACK_USERNAME, BROWSERSTACK_ACCESS_KEY. | ||
| # NOTE (App Automate): the app under test is referenced via `app: bs://...` in browserstack.yml; |
There was a problem hiding this comment.
🔴 Ties into Blocking #3: this note describes app: bs://... (re-upload if expired), but the committed browserstack.yml uses a local path that the SDK uploads each run — so this guidance doesn't match the repo. Update once the app model is settled.
| @then("search results are displayed") | ||
| def results_displayed(driver): | ||
| results = driver.find_elements(AppiumBy.CLASS_NAME, "android.widget.TextView") | ||
| assert len(results) > 0 |
There was a problem hiding this comment.
🟡 Minor: assert len(results) > 0 over all TextViews is true the moment the app opens, before any search — it won't catch a broken search. Assert on actual result rows or a known result text.
| search = driver.find_element( | ||
| AppiumBy.ID, "org.wikipedia.alpha:id/search_src_text") | ||
| search.send_keys(query) | ||
| time.sleep(5) |
There was a problem hiding this comment.
🟡 Minor: time.sleep(5) is fine for a sample, but a WebDriverWait on the results element reads better as exemplary customer code.
| @@ -0,0 +1,6 @@ | |||
| Appium-Python-Client | |||
| selenium>=3.14 | |||
| pytest==7.4.4 | |||
There was a problem hiding this comment.
🟡 Minor: pytest==7.4.4 is hard-pinned while pytest-bdd (line 4) is unpinned — a future pytest-bdd release can require a newer pytest and break this pin. Pin both or neither.
…ocument local scenario as a separate one-app-per-build run (LocalSample), reconcile README/yml-comment/CI note to the committed local-path app model, drop conftest cred duplication + fix docstring, meaningful sample assertion, unpin pytest
|
Thanks for the thorough review @shivam5643 🙏 — all valid, addressed in the latest commit. Point by point: 🔴 #1 🔴 #2 Local scenario installs the wrong app — Correct: App Automate installs one app per build, so the local scenario can't share a build with the Wikipedia/BStack sample. Fixed by making the docs honest and the scenario achievable: the README no longer claims 🔴 #3 README / yml-comment / CI note contradict the app model — Fixed. Cleaned the garbled 🟠 #4 CI only runs the android sample — Acknowledged. CI validates the android sample path; iOS + the local scenario aren't in CI (local needs its own one-app build). The docs now state what's covered rather than overclaiming. Happy to extend CI to an 🟠 #5 conftest hardcodes hub + duplicates creds — Fixed the substantive part: removed the 🟡 Minors — the android assertion now verifies results actually match the query ( (Also: the CodeQL "workflow does not contain permissions" inline comment is already resolved — the PTAL 🙏 |
3 participants
Import generated + live-validated BrowserStack SDK sample content, replacing the init scaffold. Single clean commit; no run artifacts.