Skip to content

Migrate base RulesEngineInternal operator unit tests to JSON predicate fixtures#3545

Merged
ajpallares merged 7 commits into
mainfrom
pallares/rules-engine-json-fixtures
Jun 8, 2026
Merged

Migrate base RulesEngineInternal operator unit tests to JSON predicate fixtures#3545
ajpallares merged 7 commits into
mainfrom
pallares/rules-engine-json-fixtures

Conversation

@ajpallares

@ajpallares ajpallares commented Jun 5, 2026

Copy link
Copy Markdown
Member

Android counterpart to RevenueCat/purchases-ios#6885

Motivation

Migrate the hand-written RulesEngineInternal operator unit tests into declarative JSON predicate fixtures run by one parameterized runner.

Description

  • Adds per-operator JSON fixtures (exact copies of the Migrate base RulesEngineInternal operator unit tests to JSON predicate fixtures purchases-ios#6885 fixtures) under src/test/resources/predicate-fixtures, run by a shared parameterized runner. A guard test pins the loaded count to make sure we're not missing any test case.
  • Deletes the migrated arithmetic/comparison/equality/logic suites; trims accessor/evaluator/string-array to the cases not expressible as predicate→boolean.
  • Test-only; no production changes.

Made with Cursor


Note

Low Risk
Test-only refactor with no production code changes; risk is limited to possible coverage gaps if fixtures diverge from deleted Kotlin tests.

Overview
Moves most RulesEngineInternal operator and evaluator coverage from large Kotlin unit test classes into declarative JSON fixtures under src/test/resources/predicate-fixtures/, aligned with the iOS PR.

A parameterized PredicateFixtureTest loads every fixture via PredicateConformanceFixtureLoader (kotlinx.serialization) and asserts outcomes through PredicateConformanceRunner—expected truthiness, RuleError types, and optional warning substrings. PredicateFixtureLoaderTest pins 244 loaded cases so a broken resource load cannot shrink the suite silently.

Removed standalone suites for arithmetic, comparison, equality, and logic; slimmed EvaluatorTest, AccessorOperatorsTest, and StringArrayOperatorsTest to cases that cannot be expressed as predicate→boolean fixtures (malformed JSON parse, array-scoped var, structural merge nesting, whole-data var results).

Test-only Gradle change: Kotlin serialization plugin plus kotlinx.serialization.json on the test classpath. No production rules engine code changes.

Reviewed by Cursor Bugbot for commit 53b9e92. Bugbot is set up for automated code reviews on this repo. Configure here.

…e fixtures

Adds per-operator JSON predicate fixtures (exact copies of the iOS #6885
corpus) under src/test/resources/predicate-fixtures, run by a shared
parameterized runner. Deletes the migrated arithmetic/comparison/equality/
logic suites and trims the accessor/evaluator/string-array suites to the
cases not expressible as predicate->boolean. Test-only change.

Co-authored-by: Cursor <cursoragent@cursor.com>
@codecov

codecov Bot commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.11%. Comparing base (4b64a99) to head (53b9e92).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3545   +/-   ##
=======================================
  Coverage   80.11%   80.11%           
=======================================
  Files         371      371           
  Lines       15166    15166           
  Branches     2100     2100           
=======================================
  Hits        12150    12150           
  Misses       2166     2166           
  Partials      850      850           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ajpallares and others added 5 commits June 5, 2026 17:36
Expresses the "empty-string key resolves to the whole scope and is not
missing" case as a `{"!!":{"missing":[""]}}` -> false fixture (the string
coercion the other missing fixtures use can't distinguish [] from [""]).
Removes the now-covered Kotlin test and bumps the fixture count to 237.

Co-authored-by: Cursor <cursoragent@cursor.com>
Adds fixtures for the empty-segment dot-path splits, var default-vs-null-leaf
behavior, and the non-numeric missing_some threshold (7 new cases), and drops
the now-redundant Kotlin tests. AccessorOperatorsTest keeps only the cases
that cannot be a predicate->boolean fixture (top-level array scope or a
whole-object return). Fixture count is now 244.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@ajpallares ajpallares marked this pull request as ready for review June 5, 2026 17:29
@ajpallares ajpallares requested a review from a team as a code owner June 5, 2026 17:29

@rickvdl rickvdl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! One question

expected.contains.forEach { substring ->
assertThat(warnings)
.withFailMessage("Fixture %s: missing warning containing \"%s\", got %s", id, substring, warnings)
.anyMatch { it.contains(substring) }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that we're not failing if we have the expected result + other results? In other words, is it exhaustive?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. My first iteration of this was indeed to make it exhaustive. However, I changed it intentionally. My reasoning was that if a specific test is checking that a specific warning is logged, then other logs that are independent from that test could be added/removed and that shouldn't affect that test passing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, that also makes sense. Thanks!

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

return (0 until fixtures.length()).map { parseCase(fixtures.getJSONObject(it)) }
}

private fun parseCase(json: JSONObject): PredicateConformanceFixtureCase =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could make PredicateConformanceFixtureCase serializable and use that... But TBH, I don't really mind. Happy to keep as is :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I was doubting about this but Cursor told me there wasn't really a lot of benefit. Your comment was the last push I needed, so I migrated it in 53b9e92

I also like it better like this, being more similar to iOS

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajpallares ajpallares enabled auto-merge June 8, 2026 08:38
@ajpallares ajpallares added this pull request to the merge queue Jun 8, 2026
Merged via the queue into main with commit d9929f0 Jun 8, 2026
40 checks passed
@ajpallares ajpallares deleted the pallares/rules-engine-json-fixtures branch June 8, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants