Skip to content

Commit

Permalink
Warn on missing repeats, add explicit creation when missing
Browse files Browse the repository at this point in the history 
Followup from [this discussion](#150 (comment))

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.
  • Loading branch information
@eyelidlessness
eyelidlessness committed Jul 16, 2024
1 parent a933f6f commit 2cac0d4
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 33 deletions.
84 changes: 84 additions & 0 deletions packages/scenario/src/jr/Scenario.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ interface CreateNewRepeatAssertedReferenceOptions {
readonly assertCurrentReference: string;
}

export interface ExplicitRepeatCreationOptions {
readonly explicitRepeatCreation: boolean;
}

// prettier-ignore
type GetQuestionAtIndexParameters<
ExpectedQuestionType extends QuestionNodeType
Expand Down Expand Up @@ -304,6 +308,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.`
);
Expand Down Expand Up @@ -655,6 +661,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**
*
Expand Down
84 changes: 51 additions & 33 deletions packages/scenario/test/select.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<ExplicitRepeatCreationOptions>([
{ 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);
});
});
});

Expand Down

0 comments on commit 2cac0d4

Please sign in to comment.