fix(test builder): add toggle case to per-method entity test generator (#289) - #290
Conversation
#289) PR #287 added the entity-methods default [get, update, toggle] to both di_plugin.dart and test_plugin.dart so canonical zfa make invocations route through per-method generation. Every other generator that switches on method already had a toggle case (usecase, controller, presenter, view, repository, datasource, di, mock) EXCEPT the per-method test builder (test_builder_entity.dart). Its switch covered get/getList/list /create/update/delete/watch/watchList but not toggle, so the test plugin crashed with ArgumentError: Unknown method: toggle before emitting any file. The two test-builder helpers (_getFallbackValues and _generateFutureTests in test_builder_helpers.dart) also had no toggle branch, so even if the switch were relaxed the emitted test file would have been empty. Fix: add case toggle to test_builder_entity.dart (className = Toggle${entityName}UseCase, returnTypeConstructor = t$entityName, isStream/isCompletable = false — mirrors the usecase generator) and add matching toggle branches to _getFallbackValues (registerFallbackValue gets a concrete ToggleParams<idType, ${entityName}Fields> instance) and _generateFutureTests (paramsExpr + arrangeCall/verifyCall as mockVarName.toggle(any())) in test_builder_helpers.dart. Also adds a regression test in test/integration/toggle_method_test.dart that exercises the exact failing config from the issue (methods: [get, update, toggle] + all generation flags + generateTest: true) and asserts the toggle usecase test file is emitted with the expected shape (ToggleTodoUseCase class, mockRepository.toggle call, ToggleParams<String, TodoFields> constructor, TodoFields.id field reference). Closes #289.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe test plugin now supports ChangesToggle test generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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. Comment |
arrrrny
left a comment
arrrrny
left a comment
There was a problem hiding this comment.
Reviewing PR #290: Fix toggle case in test builder.
Walkthrough
This PR adds the missing 'toggle' method case to the test builder, ensuring that ❌ Usage: zfa make ... [options]
Example: zfa make User route di with the flag succeeds when the toggle method is involved. It includes regression testing to verify the fix and prevent future regressions.
Changes Table
| Layer / File(s) | Summary |
|---|---|
| Added to method switch. | |
| Updated and to support . | |
| Added new regression test case. |
Pre-merge Checks
- Code compiles with the fix.
- Regression tests pass.
- Mirrors existing toggle support in other generators.
Verdict
The fix is surgical, well-reasoned, and correctly aligns the test builder with the rest of the generator system. LGTM.
A rabbit hops in, checks the code, and finds the toggle path on the toadstool. All is well.
WalkthroughNote A focused, surgical fix for #289: the per-method test builder never got a Verified locally:
Changes
Estimated Review Effort: 2 (15 minutes)
Pre-Merge Checks
🧸 Poetry |
arrrrny
left a comment
arrrrny
left a comment
There was a problem hiding this comment.
Actionable comments posted: 2
- 🎯 Functional Correctness · 🟠 Major · 🔨 Medium —
lib/src/plugins/test/builders/test_builder_helpers.dart:104— emitted toggle test file does not typecheck (field: TodoFields.idis aField, not aTodoFields) - 🧪 Tests · 🟡 Minor · ⚡ Quick win —
test/integration/toggle_method_test.dart:219— regression test never compiles the generated test file
Nitpicks
🔵 Trivial —
test/integration/toggle_method_test.dart:210— the hardcoded'$outputDir/../../test/...'path magic couples the test to the workspace layout; derive it from the sameoutputDir.replaceAll('lib/src', '')computation the builder uses so the two can't silently drift apart.
Prompt for all review comments
🤖 Prompt for AI Agents
1. lib/src/plugins/test/builders/test_builder_helpers.dart:104 — ToggleParams<String, TodoFields>(field: TodoFields.id)
does not typecheck: TodoFields.id is a static const Field<Todo, String>, but the field parameter has type
F = TodoFields (abstract final class, no instances). Align the ToggleParams F type argument with the
field-descriptor type consumed by the datasource (e.g. Field<Todo, bool>) across usecase/presenter/datasource/
test builders, then dart analyze a generated workspace with methods [get, update, toggle].
2. test/integration/toggle_method_test.dart:219 — add a runDartAnalyze(workspace, [test file]) assertion (exit 0)
so the emitted test is compile-verified; string-contains checks pass even when the file doesn't typecheck.
3. test/integration/toggle_method_test.dart:210 — derive the test file path from outputDir.replaceAll('lib/src', '')
instead of the hardcoded '../..' to avoid drift between the test and the builder's path computation.
Verdict
The fix correctly resolves the crash (generation now succeeds with the test plugin + toggle), the switch alignment matches every other generator, and the regression test locks in the core behavior. However, the emitted toggle test file does not typecheck — the
Ftype argument ofToggleParamsis inconsistent with the values the whole stack produces — and the regression test only string-sniffs the output, so this ships silently. Recommend addressing the type-level inconsistency (or at minimum compile-verifying the generated file) before merge. The pre-existing CI failures are unrelated infra (missing../zorphy/zorphy_annotationpath dependency). 🐰
| 'ToggleParams<$idType, ${entityName}Fields>', | ||
| ).call([], { | ||
| 'id': idValue, | ||
| 'field': refer('${entityName}Fields').property(config.queryField), |
There was a problem hiding this comment.
🎯 Functional Correctness · 🟠 Major · 🔨 Medium
Emitted toggle test file does not typecheck — field: TodoFields.id is a Field, not a TodoFields
The toggle branch constructs ToggleParams<String, ${entityName}Fields>(id: ..., field: ${entityName}Fields.<queryField>, value: true) (here and again in _getFallbackValues at line 104). ToggleParams<I, F> declares field as type F, and the whole generated stack instantiates it as F = ${entityName}Fields. But ${entityName}Fields.id is a static const Field<Entity, IdType> property (see zorphy's fields_class_generator.dart and real generated entities), and abstract final class TodoFields has no instances — so no value of type TodoFields can ever be constructed.
dart analyze on a faithful minimal repro with the real ToggleParams and a real-shaped TodoFields fails:
error - The argument type 'Field<Todo, String>' can't be assigned to the parameter type 'TodoFields'.
Impact: in a real Flutter project, zfa make <Entity> --preset=crud --with=vpc,state,di,test with toggle in the methods list now succeeds (no crash — good) but emits toggle_<entity>_usecase_test.dart that fails dart analyze. The crash is replaced by silently-broken generated code.
Note this mirrors a latent inconsistency in the pre-existing toggle stack (the presenter also types field as ${entityName}Fields while the datasource consumes params.field as a Field via existing.copyWithField(params.field, ...)) — so the root fix is upstream: align ToggleParams's F parameter with the actual field-descriptor type (e.g. Field<Entity, bool>), or provide a constructible field value. Within this PR's scope, at minimum the emitted test should not reference ${entityName}Fields.<queryField> as a value.
Proposed fix
The cleanest fix is to stop treating the Fields class as a value type. Either:
- Change
ToggleParams<I, F>'sFto the field descriptor type and generateToggleParams<String, Field<Todo, bool>>(field: TodoFields.isCompleted, ...)across usecase/presenter/datasource/test — a broader, consistent fix; or - In the test builder only, construct the params via a value that actually typechecks (e.g. a mocktail-safe placeholder of the correct type).
Because abstract final class TodoFields has no instances, option 2 is impossible without a core change — so this needs the upstream alignment (option 1). Verify with dart analyze on a generated workspace's test/domain/usecases/ after the change.
🤖 Prompt for AI Agents
In lib/src/plugins/test/builders/test_builder_helpers.dart (toggle branches in
_getFallbackValues and _generateFutureTests), the emitted
ToggleParams<String, TodoFields>(field: TodoFields.id, ...) does not typecheck:
TodoFields.id is a static const Field<Todo, String>, but the field parameter
has type F = TodoFields, an abstract final class with no instances. Align the
ToggleParams F type argument with the field-descriptor type actually consumed
(e.g. Field<Todo, bool>) across the usecase generator, presenter, datasource,
and test builders, then verify by generating a workspace with methods
[get, update, toggle] and running `dart analyze` on the emitted
test/domain/usecases/** files.
| reason: 'toggle_todo_usecase_test.dart should be generated', | ||
| ); | ||
|
|
||
| final content = toggleTestFile.readAsStringSync(); |
There was a problem hiding this comment.
🧪 Tests · 🟡 Minor · ⚡ Quick win
Regression test never compiles the generated test file — it can't catch the type error above
The new test asserts result.success, file existence, and four contains(...) string checks against the emitted test file. It never runs dart analyze (or dart compile) on the generated workspace, so the emitted toggle_todo_usecase_test.dart could be full of type errors and this test would still pass — which is exactly what happens today (see the major finding). The test name promises "no Unknown method: toggle crash", and it verifies that, but the deliverable is a compilable test file, and that property is unverified.
The test utilities already expose runDartAnalyze(workspace, [...paths]) (regression_test_utils.dart) — wire it in so generation output is compile-verified, not just string-sniffed.
Proposed fix
After the expect(result.success, isTrue, ...) block, add:
final analyze = await runDartAnalyze(workspace, [
'test/domain/usecases/todo/toggle_todo_usecase_test.dart',
]);
expect(analyze.exitCode, 0, reason: analyze.stdout);(plus the equivalent dart analyze check on the lib/src output, which would also surface the pre-existing copyWithField gap in the generated local datasource).
🤖 Prompt for AI Agents
In test/integration/toggle_method_test.dart, the new #289 regression test
only asserts string containment on the emitted test file. Add a
runDartAnalyze(workspace, ['test/domain/usecases/todo/toggle_todo_usecase_test.dart'])
assertion (exit code 0) so generated test output is compile-verified; the
current suite passes even though the emitted toggle test file does not
typecheck. See regression_test_utils.dart for the helper.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Closes #289
Summary
Fixes #289 —
zfa makecrashed withUnknown method: togglewhenever an entity make used the default entity-methods default (introduced by #287) together with the test plugin (--with=test).Root cause
PR #287 added the entity-methods default
[\"get\", \"update\", \"toggle\"]to bothdi_plugin.dartandtest_plugin.dartso canonicalzfa make <Entity> --preset=crud --with=vpc,state,di,testinvocations route through per-method generation. Every other generator that switches onmethodalready had acase \"toggle\":…EXCEPT the per-method test builder (
test_builder_entity.dart) — itsswitch (method)covered get / getList / list / create / update / delete / watch / watchList but not toggle, so the test plugin crashed withArgumentError: Unknown method: togglebefore emitting any file. The two test-builder helpers (_getFallbackValuesand_generateFutureTestsintest_builder_helpers.dart) also had no toggle branch, so even if the switch were relaxed the emitted test file would have been empty.Fix
Purely additive — three files, +111 lines, no deletions:
lib/src/plugins/test/builders/test_builder_entity.dart— addcase \"toggle\":to the method switch. Mirrors the usecase generator:Toggle\${entityName}UseCase, returns the toggled entity (Future<Entity>), soreturnTypeConstructor = \"t\$entityName\",isStream = false,isCompletable = false. The usecase-file-name logic falls through to the standard${method}_${entitySnake}_usecase.dartpattern, so the emitted test file is namedtoggle_<entity>_usecase_test.dart.lib/src/plugins/test/builders/test_builder_helpers.dart:_getFallbackValues: addcase \"toggle\":returningToggleParams<idType, \${entityName}Fields>(id: idValue, field: \${entityName}Fields.<queryField>, value: true)soregisterFallbackValuereceives a concreteToggleParamsmatching the usecase generator signature._generateFutureTests: addelse if (method == \"toggle\")branch constructing the sameToggleParamsand callingmockVarName.toggle(any())for both arrange and verify (mirrors the update branch).test/integration/toggle_method_test.dart— new regression test case that exercises the exact failing config from the issue (methods: [get, update, toggle]+ all generation flags +generateTest: true) and asserts:Unknown method: toggle),toggle_<entity>_usecase_test.dartis emitted,Toggle<Entity>UseCase,mockRepository.toggle(,ToggleParams<String, <Entity>Fields>, and<Entity>Fields.id.Why option 1 from the issue (add toggle case) vs. option 2 (drop toggle from defaults) vs. option 3 (shared registry)
Verification
dart analyze lib/: 1 pre-existing info-level lint indecorator_dispatcher.dart(unrelated, not touched). Test plugin subtree: clean.dart test test/integration/toggle_method_test.dart: 2/2 pass.dart test test/integration/ test/commands/make_command_test.dart: 32/32 pass.dart test test/commands/test_command_test.dart test/core/plugin_system/plan_store_test.dart: 10/10 pass.dart test test/integration/full_entity_workflow_test.dart test/integration/di_flag_parsing_test.dart test/integration/sync_with_di_test.dart: 3/3 pass.dart test test/regression/: 34/34 pass.Reproduction
Before this fix, the exact commands from the issue crash:
After this fix, the same commands complete successfully and emit (among others):
test/domain/usecases/product/toggle_product_usecase_test.darttest/domain/usecases/product/get_product_usecase_test.darttest/domain/usecases/product/update_product_usecase_test.dartSummary by CodeRabbit
New Features
Tests