From db711a894d6c9ef646c7e0d0f327faad6bafaff6 Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Thu, 4 Jul 2024 12:20:38 -0700 Subject: [PATCH] Explore: warn on missing repeats, add explicit creation when missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Followup from [this discussion](https://github.com/getodk/web-forms/pull/150#discussion_r1665960350) The idea here is: 1. Explicit repeat creation in tests will improve test clarity 2. Introduce a clear way to make similar changes in JavaRosa as they come up 3. Detect missing repeats (with a still naive approach[^1], albeit now recursive) and **log with a stack trace** so explicit calls can be introduced (conditionally, with parameterization like many other cases where we make adjustments to the JavaRosa direct port) 4. Add a new proposed `Scenario` method which… - Makes clear where explicit repeat creation calls are added, in a way that can be traced directly in test source, whenever convenient - Assumes the call occurs in such a sub-suite parameterizing whether to explicitly add repeats as detected; adds repeats as explicitly specified in the true condition, suppresses logging in the false condition This approach already detected one test which would have passed if adding repeats had been explicit. The test is updated here to demonstrate that. Notice that the test’s **PORTING NOTES** have also been removed. This is because the notes were wrong! This is an excellent example of how misleading it is that tests fail for lack of this implicit behavior! The actual test logic is not substantially noisier or more complex as a result. This feels like a clear win to me. [^1]: Keeping this naive seems fine for the limited scope of usage. The reference expressions which reach this point are limited to `Scenario.answer` calls with an explicit reference. If we’re using references of arbitrary complexity in those calls, I think we’ve got much bigger problems than this functionality being so narrowly scoped. --- packages/scenario/src/jr/Scenario.ts | 84 +++++++++++++++++++++++++++ packages/scenario/test/select.test.ts | 84 ++++++++++++++++----------- 2 files changed, 135 insertions(+), 33 deletions(-) diff --git a/packages/scenario/src/jr/Scenario.ts b/packages/scenario/src/jr/Scenario.ts index 61cccb65..78670ea2 100644 --- a/packages/scenario/src/jr/Scenario.ts +++ b/packages/scenario/src/jr/Scenario.ts @@ -64,6 +64,10 @@ interface CreateNewRepeatAssertedReferenceOptions { readonly assertCurrentReference: string; } +export interface ExplicitRepeatCreationOptions { + readonly explicitRepeatCreation: boolean; +} + // prettier-ignore type GetQuestionAtIndexParameters< ExpectedQuestionType extends QuestionNodeType @@ -295,6 +299,8 @@ export class Scenario { }); if (index === -1) { + this.logMissingRepeatAncestor(reference); + throw new Error( `Setting answer to ${reference} failed: could not locate question/positional event with that reference.` ); @@ -634,6 +640,84 @@ export class Scenario { return event.eventType === 'END_OF_FORM'; } + private suppressMissingRepeatAncestorLogs = false; + + private logMissingRepeatAncestor(reference: string): void { + if (this.suppressMissingRepeatAncestorLogs) { + return; + } + + const [, positionPredicatedReference, positionExpression] = + reference.match(/^(.*\/[^/[]+)\[(\d+)\]\/[^[]+$/) ?? []; + + if (positionPredicatedReference == null || positionExpression == null) { + return; + } + + if (/\[\d+\]/.test(positionPredicatedReference)) { + this.logMissingRepeatAncestor(positionPredicatedReference); + } + + const position = parseInt(positionExpression, 10); + + if (Number.isNaN(position) || position < 1) { + throw new Error( + `Cannot log missing repeat ancestor for reference (invalid position predicate): ${reference} (repeatRangeReference: ${positionPredicatedReference}, positionExpression: ${positionExpression})` + ); + } + + try { + const ancestorNode = this.getInstanceNode(positionPredicatedReference); + + if (ancestorNode.nodeType !== 'repeat-range') { + // eslint-disable-next-line no-console + console.trace( + 'Unexpected position predicate for ancestor reference:', + positionPredicatedReference, + 'position:', + position + ); + + return; + } + + const index = position - 1; + const repeatInstances = ancestorNode.currentState.children; + + if (repeatInstances[index] == null) { + // eslint-disable-next-line no-console + console.trace( + 'Missing repeat in range:', + positionPredicatedReference, + 'position:', + position, + 'index:', + index, + 'actual instances present:', + repeatInstances.length + ); + } + } catch (error) { + // eslint-disable-next-line no-console + console.error(error); + + return; + } + } + + proposed_addExplicitCreateNewRepeatCallHere( + reference: string, + options: ExplicitRepeatCreationOptions + ): unknown { + if (options.explicitRepeatCreation) { + return this.createNewRepeat(reference); + } + + this.suppressMissingRepeatAncestorLogs = true; + + return; + } + /** * **PORTING NOTES** * diff --git a/packages/scenario/test/select.test.ts b/packages/scenario/test/select.test.ts index 0adf62f1..68533ec8 100644 --- a/packages/scenario/test/select.test.ts +++ b/packages/scenario/test/select.test.ts @@ -19,6 +19,7 @@ import { describe, expect, it } from 'vitest'; import { answerText } from '../src/answer/ExpectedDisplayTextAnswer.ts'; import { stringAnswer } from '../src/answer/ExpectedStringAnswer.ts'; import { choice } from '../src/choice/ExpectedChoice.ts'; +import type { ExplicitRepeatCreationOptions } from '../src/jr/Scenario.ts'; import { Scenario } from '../src/jr/Scenario.ts'; import type { PositionalEvent } from '../src/jr/event/PositionalEvent.ts'; import { setUpSimpleReferenceManager } from '../src/jr/reference/ReferenceManagerTestUtils.ts'; @@ -376,49 +377,66 @@ describe('DynamicSelectUpdateTest.java', () => { }); }); - /** - * **PORTING NOTES** - * - * This currently fails because repeat-based itemsets are broken more - * generally. As with the above sub-suite, the last assertion is a reference - * check and will always pass. Once repeat-based itemsets are fixed, we'll - * want to consider whether this test should be implemented differently too. - */ describe('select with repeat as trigger', () => { - it.fails('recomputes [the] choice list at every request', async () => { - const scenario = await Scenario.init( - 'Select with repeat trigger', - html( - head( - title('Repeat trigger'), - model( - mainInstance(t("data id='repeat-trigger'", t('repeat', t('question')), t('select'))), + describe.each([ + { explicitRepeatCreation: false }, + { explicitRepeatCreation: true }, + ])('explicit repeat creation: $explicitRepeatCreation', ({ explicitRepeatCreation }) => { + let testFn: typeof it | typeof it.fails; + + if (explicitRepeatCreation) { + testFn = it; + } else { + testFn = it.fails; + } - instance('choices', item('1', 'A'), item('2', 'AA'), item('3', 'B'), item('4', 'BB')) - ) - ), - body( - repeat('/data/repeat', input('/data/repeat/question')), - select1Dynamic( - '/data/select', - "instance('choices')/root/item[value>count(/data/repeat)]" + testFn('recomputes [the] choice list at every request', async () => { + const scenario = await Scenario.init( + 'Select with repeat trigger', + html( + head( + title('Repeat trigger'), + model( + mainInstance( + t("data id='repeat-trigger'", t('repeat', t('question')), t('select')) + ), + + instance( + 'choices', + item('1', 'A'), + item('2', 'AA'), + item('3', 'B'), + item('4', 'BB') + ) + ) + ), + body( + repeat('/data/repeat', input('/data/repeat/question')), + select1Dynamic( + '/data/select', + "instance('choices')/root/item[value>count(/data/repeat)]" + ) ) ) - ) - ); + ); + + scenario.answer('/data/repeat[1]/question', 'a'); - scenario.answer('/data/repeat[1]/question', 'a'); + expect(scenario.choicesOf('/data/select').size()).toBe(3); - expect(scenario.choicesOf('/data/select').size()).toBe(3); + scenario.proposed_addExplicitCreateNewRepeatCallHere('/data/repeat', { + explicitRepeatCreation, + }); - scenario.answer('/data/repeat[2]/question', 'b'); + scenario.answer('/data/repeat[2]/question', 'b'); - const choices = scenario.choicesOf('/data/select'); + const choices = scenario.choicesOf('/data/select'); - expect(choices.size()).toBe(2); + expect(choices.size()).toBe(2); - // Because of the repeat trigger in the count expression, choices should be recomputed every time they're requested - expect(scenario.choicesOf('/data/select')).not.toBe(choices); + // Because of the repeat trigger in the count expression, choices should be recomputed every time they're requested + expect(scenario.choicesOf('/data/select')).not.toBe(choices); + }); }); });