Add tests for frequency-based trips

[akimovpro]

Summary

Add tests for frequency-based trips using USF Bull Runner data.

• Mock JSON response with frequency fields (startTime, endTime, headway)
• 3 integration tests: parse frequency trips, headway-to-minutes conversion, null frequency for schedule-based trips
• 3 unit tests: Jackson deserialization of frequency (present, null, missing)

Fixes #430

Test plan

• Run gradlew connectedObaGoogleDebugAndroidTest – all new and existing tests pass

---

• Apply the AndroidStyle.xml style template to your code in Android Studio.
• Run the unit tests with gradlew connectedObaGoogleDebugAndroidTest to make sure you didn't break anything
• If you have multiple commits please combine them into one commit by squashing them for the initial submission of the pull request.

Summary by CodeRabbit

• Tests
  • Expanded test coverage for frequency-based arrivals, validating headway, start/end times, and predicted status
  • Added tests ensuring schedule-based trips do not populate frequency
  • New UI test verifying frequency status labels render correctly with locale-aware time formatting
  • Added test fixtures and request mapping to cover frequency-based arrival scenarios

[CLAassistant]

All committers have signed the CLA.

[aaronbrethorst]

Hi Igor & Sviatoslav,

Thanks for picking up issue #430 — frequency-based trips have been a coverage gap for a long time and the USF Bull Runner data is a great fit for it. The new Frequency Jackson deserialization tests in ObaArrivalInfoTest.java are nicely scoped, the URI map wiring is correct, and reusing the existing HART_3105 fixture for the negative case (rather than adding another one) is the right instinct.

That said, I'd like a few changes before merging. The headline issue is that the PR closes #430 in letter but not in spirit: none of the new tests exercise the production code that actually renders frequency-based arrivals to the user. There's also one test I'd ask you to delete outright.

Request Changes

Critical Issues (1 found)

• testFrequencyHeadwayAsMinutes is tautological — please delete it. [ArrivalInfoRequestTest.java:763-780] The test re-fetches the same fixture as testFrequencyBasedTrips, then re-implements (int) (freq.getHeadway() / 60) inline in the test body and asserts the result equals 10. The fixture sets headway to 600, so this is really assertEquals(10, 600 / 60) — Java math, not a behavior test. If ArrivalInfo.java:155 were changed tomorrow to headway / 30, this test would still pass. It also doesn't add coverage that testFrequencyBasedTrips doesn't already give us via assertEquals(600, arrival0.getFrequency().getHeadway()) at line 742. Either remove it, or replace it with a test that calls the actual production code path (see next item).

Important Issues (2 found)

• No test covers the production rendering of frequency arrivals. The whole reason #430 was filed is that the user-visible "Every N mins from HH:MM" / "Every N mins until HH:MM" output at ArrivalInfo.java:152-171 is unverified. As written, this PR proves we parse frequency JSON, but a regression that swapped the from/until labels, lost the seconds-to-minutes conversion, or short-circuited the whole branch would pass every test in this PR. UIUtilTest.java is the natural home — it already exercises getStatusText() for delay, on-time, and canceled cases. Please add at least one test there that asserts the rendered label for a frequency arrival using the USF fixture.

• Both branches of the now < frequency.getStartTime() check should be covered. Once you're testing getStatusText(), please cover both stop_info_frequency_from and stop_info_frequency_until — they're independent strings and either one could regress silently. The USF fixture's currentTime of 1456780082393 falls between startTime and endTime, so it'll exercise the "until" branch naturally; a second case with a now before startTime covers "from".

Fit and Finish (3 found)

• Redundant setCustomApiUrl("api.pugetsound.onebusaway.org") calls. [ArrivalInfoRequestTest.java:717, :766] ObaTestCase.before() already sets this exact URL before every test (see ObaTestCase.java:58). The other tests in the file only call setCustomApiUrl when they're overriding the default with a different region. Drop these two lines. The Tampa URL override at line 785 in testScheduleBasedTripHasNullFrequency is legitimate — keep that one.

• Weak nearby-stops assertion. [ArrivalInfoRequestTest.java:760] assertTrue(nearbyStops.size() > 0) doesn't pin much down when the fixture is deterministic. The USF fixture contains exactly one nearby stop (USF Bull Runner_204, "Research & Development"). Please tighten to assertEquals(1, nearbyStops.size()) and check the ID, matching the precision of the other assertions in the same test.

• Add a length guard before indexing in testScheduleBasedTripHasNullFrequency. [ArrivalInfoRequestTest.java:793-794] You assert arrivals != null and then immediately index arrivals[0]. If the array were empty, you'd get AIOOBE instead of a clean failure message. Either assertTrue(arrivals.length > 0) or assertEquals(1, arrivals.length) before indexing — consistent with the style in testFrequencyBasedTrips.

Strengths

• The three testFrequency_* Jackson tests in ObaArrivalInfoTest.java (present / null / missing) are clean, focused unit tests at the right level — they're the part of the PR I'd land as-is.
• testFrequencyBasedTrips proves the full request → response → Frequency chain end-to-end and asserts every field of Frequency, which is the genuine new coverage.
• The fixture is realistic two-route data lifted straight from the issue, and the urimap.json entry follows the established pattern.

Recommended Action

1. Delete testFrequencyHeadwayAsMinutes.
2. Add at least one test of ArrivalInfo.getStatusText() (in UIUtilTest.java) that exercises the frequency-based rendering path — ideally one each for the "from" and "until" branches.
3. Drop the two redundant setCustomApiUrl(...) calls in the USF tests.
4. Tighten the nearbyStops assertion and add the array-length guard.

Happy to take another look once these are in — the bones of this PR are good, it just needs to reach the code path that actually matters to riders.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ef60151b-5ae0-40f7-8dbd-458831060286

📥 Commits

Reviewing files that changed from the base of the PR and between b3f11cc and eafb167.

📒 Files selected for processing (5)

• onebusaway-android/src/androidTest/java/org/onebusaway/android/io/test/ArrivalInfoRequestTest.java
• onebusaway-android/src/androidTest/java/org/onebusaway/android/io/test/ObaArrivalInfoTest.java
• onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/UIUtilTest.java
• onebusaway-android/src/androidTest/res/raw/arrivals_and_departures_for_stop_usf_frequency.json
• onebusaway-android/src/androidTest/res/raw/urimap.json

✅ Files skipped from review due to trivial changes (2)

• onebusaway-android/src/androidTest/res/raw/urimap.json
• onebusaway-android/src/androidTest/res/raw/arrivals_and_departures_for_stop_usf_frequency.json

🚧 Files skipped from review as they are similar to previous changes (2)

• onebusaway-android/src/androidTest/java/org/onebusaway/android/io/test/ArrivalInfoRequestTest.java
• onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/UIUtilTest.java

---

📝 Walkthrough

Walkthrough

Adds a new JSON fixture and URI mapping plus unit/instrumentation/UI tests that validate frequency field deserialization, API responses for frequency vs schedule-based trips, and localized frequency status label rendering.

Changes

Frequency-Based Trip Test Coverage

Layer / File(s)	Summary
Test fixture setup onebusaway-android/src/androidTest/res/raw/arrivals_and_departures_for_stop_usf_frequency.json, onebusaway-android/src/androidTest/res/raw/urimap.json	New JSON fixture with frequency data for USF Bull Runner stop #203 (headway 600 seconds, start/end window) and URI-to-fixture mapping for the mock endpoint.
Frequency field deserialization onebusaway-android/src/androidTest/java/org/onebusaway/android/io/test/ObaArrivalInfoTest.java	Three test methods verify ObaArrivalInfo#getFrequency() returns a populated object when present, and null when frequency is explicitly null or missing.
Frequency-based trip integration onebusaway-android/src/androidTest/java/org/onebusaway/android/io/test/ArrivalInfoRequestTest.java	Two instrumentation tests validate frequency-based arrivals (headway, start/end timestamps, non-predicted flag, nearby stops) and that a schedule-based trip returns null frequency.
Frequency status label UI rendering onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/UIUtilTest.java	Adds imports and a test that asserts ArrivalInfo#getStatusText() produces localized "Every N mins" labels with correct "until"/"from" phrasing depending on current time relative to the frequency window.

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly reflects the main change: adding tests for frequency-based trips, which matches the comprehensive test additions across multiple test files.
Linked Issues check	✅ Passed	The PR fully addresses Issue #430 requirements: adds unit tests for frequency deserialization (present/null/missing), integration tests for frequency-based trips, tests schedule-based trip null frequency, and uses USF Bull Runner fixture with frequency fields.
Out of Scope Changes check	✅ Passed	All changes are directly related to testing frequency-based trips: new test methods, test fixtures, and URI mappings are all necessary for the stated objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[akimovpro]

Thanks for the thorough review — really helpful framing on "letter vs. spirit" of #430. I've pushed an update addressing everything:

Critical

Deleted testFrequencyHeadwayAsMinutes — agreed it was tautological (assertEquals(10, 600 / 60)) and added no coverage over the field assertions already in testFrequencyBasedTrips.

Important
Added testFrequencyStatusLabels in UIUtilTest, which now exercises the actual rendering path via ArrivalInfo.getStatusText() on the USF fixture, not just JSON parsing.
Covered both branches of the now < frequency.getStartTime() check:
stop_info_frequency_until — using the fixture's currentTime (which falls inside the frequency window);
stop_info_frequency_from — using a now set before startTime.
The expected label is built with the same DateFormat.getTimeInstance(SHORT) + getString(...) that production uses, so the assertion stays locale/timezone-independent rather than hardcoding a clock string.

Fit and Finish

Dropped the two redundant setCustomApiUrl("api.pugetsound.onebusaway.org") calls in the USF tests (kept the Tampa override in the schedule-based test).
Tightened the nearby-stops assertion to assertEquals(1, nearbyStops.size()) plus an ID check on USF Bull Runner_204.
Added an assertTrue(arrivals.length > 0) guard before indexing in testScheduleBasedTripHasNullFrequency.