diff --git a/packages/scenario/src/answer/UntypedAnswer.ts b/packages/scenario/src/answer/UntypedAnswer.ts index a1a7c142..36aa1a9a 100644 --- a/packages/scenario/src/answer/UntypedAnswer.ts +++ b/packages/scenario/src/answer/UntypedAnswer.ts @@ -1,3 +1,4 @@ +import type { Scenario } from '../jr/Scenario.ts'; import { ComparableAnswer } from './ComparableAnswer.ts'; interface MaybeStringable { @@ -18,18 +19,53 @@ const isExplicitlyCastableToString = (value: unknown): value is Stringable => { }; /** - * @todo This was previously implemented as a `castToString` function. Its call - * site suggested that this casting didn't belong in the test interface, instead - * perhaps belonging in the engine. That seems less likely now, as the client - * interface has intentionally evolved to associate specific runtime value - * encodings with a given node type; these runtime encoding types are not now, - * nor planned to be, mapped to JavaRosa's equivalents (if/when those exist). So - * it seems likely this is actually a more stable way to handle bridging the - * `Scenario` interface (presently its `answer` method, possibly more to come?) - * than trying to convolute such casting into the design of the engine's client - * interface directly. This is only presented as a `@todo` to draw attention in - * review. If we agree on this reasoning, we can likely revise this comment to - * remove reference to the previous reasoning and remove that `@todo` tag. + * **PORTING NOTES** + * + * Initial thoughts (preserved intact for posterity, edited to strike the last + * bit which now feels less clear): + * + * > This was previously implemented as a `castToString` function. Its call site + * > suggested that this casting didn't belong in the test interface, instead + * > perhaps belonging in the engine. That seems less likely now, as the client + * > interface has intentionally evolved to associate specific runtime value + * > encodings with a given node type; these runtime encoding types are not now, + * > nor planned to be, mapped to JavaRosa's equivalents (if/when those exist). + * > So it seems likely this is actually a more stable way to handle bridging + * > the `Scenario` interface (presently its `answer` method, possibly more to + * > come?) than trying to convolute such casting into the design of the + * > engine's client interface directly. ~~This is only presented as a `@todo` + * > to draw attention in review. If we agree on this reasoning, we can likely + * > revise this comment to remove reference to the previous reasoning and + * > remove that `@todo` tag.~~ + * + * Further thought on reflection of some surprises in the first + * `relevant`-focused test ported from `TriggerableDagTest.java`: + * + * While exploring some of the contours of the test's failure modes, it became + * more clear that there are a few intersecting concerns that we'll need to + * address at least in the engine interface, and which have implications for + * if/how we handle casting in `scenario` (whether in JavaRosa ported tests, or + * future tests using pertinent aspects of the same {@link Scenario} interface): + * + * - Evaluating an XPath expression to `boolean` has slightly different + * semantics in [ODK] XForms than in XPath alone: a bound node's `` + * is expected to influence casting behavior of reads, at least within certain + * semantic contexts (like `relevant`). This is discussed in more detail in + * porting notes on the test itself; these notes are added to ensure that we + * don't lose track of the implications in the next point... + * + * - A bound node's `` **may** be expected to influence casting when + * writing values to those bound nodes, though that may turn out to be moot + * when the read semantics are sufficiently addressed. We'll certainly have + * more clarity on the point as we address the pertinent features and any + * associated defects. In any case, this abstraction now feels less like a + * "sure thing" as a consequence of that. + * + * - A brief local exploration, not included in this commit, revealed that + * slightly modifying the test in question to produce semantics closer to + * XPath **still implicated** ``-specific casting as a potential + * concern, particularly around falsy values (which is to be expected, because + * XPath `boolean` casting semantics are themselves complex). */ export class UntypedAnswer extends ComparableAnswer { constructor(readonly unknownValue: unknown) { diff --git a/packages/scenario/src/jr/Scenario.ts b/packages/scenario/src/jr/Scenario.ts index 9d6be707..0a3e5340 100644 --- a/packages/scenario/src/jr/Scenario.ts +++ b/packages/scenario/src/jr/Scenario.ts @@ -8,7 +8,7 @@ import { SelectValuesAnswer } from '../answer/SelectValuesAnswer.ts'; import { answerOf } from '../client/answerOf.ts'; import type { TestFormResource } from '../client/init.ts'; import { initializeTestForm } from '../client/init.ts'; -import { getClosestRepeatRange } from '../client/traversal.ts'; +import { getClosestRepeatRange, getNodeForReference } from '../client/traversal.ts'; import { UnclearApplicabilityError } from '../error/UnclearApplicabilityError.ts'; import type { EndOfFormEvent } from './event/EndOfFormEvent.ts'; import { PositionalEvent } from './event/PositionalEvent.ts'; @@ -324,6 +324,24 @@ export class Scenario { return answerOf(this.instanceRoot, reference); } + /** + * **PORTING NOTES** + * + * Should we consider a more general name for this? It returns any node type, + * not just nodes which may be considered an "answer" to a "question". For + * instance, the first assertion in the first test ported test calling it + * checks the relevance of a group. + */ + getAnswerNode(reference: string): AnyNode { + const node = getNodeForReference(this.instanceRoot, reference); + + if (node == null) { + throw new Error(`No "answer" node for reference: ${reference}`); + } + + return node; + } + choicesOf(reference: string): SelectChoiceList { const events = this.getPositionalEvents(); // TODO: generalize more lookups... diff --git a/packages/scenario/test/relevant.test.ts b/packages/scenario/test/relevant.test.ts new file mode 100644 index 00000000..fa0d063d --- /dev/null +++ b/packages/scenario/test/relevant.test.ts @@ -0,0 +1,223 @@ +import { + bind, + body, + group, + head, + html, + input, + mainInstance, + model, + t, + title, +} from '@odk-web-forms/common/test/fixtures/xform-dsl/index.ts'; +import { describe, expect, it } from 'vitest'; +import type { UntypedAnswer } from '../src/answer/UntypedAnswer.ts'; +import { Scenario } from '../src/jr/Scenario.ts'; + +describe('Relevance - TriggerableDagTest.java', () => { + /** + * Non-relevance is inherited from ancestor nodes, as per the W3C XForms specs: + * - https://www.w3.org/TR/xforms11/#model-prop-relevant + * - https://www.w3.org/community/xformsusers/wiki/XForms_2.0#The_relevant_Property + */ + + describe('non-relevance', () => { + /** + * **PORTING NOTES** + * + * - This fails because the `relevant` expressions produce node-sets, which + * will always evaluate to `true` when those nodes are present (which they + * always are in this test). + * + * - Those node-sets evaluate to nodes which are bound with ``, which strongly suggests that a bind's data type + * should influence casting semantics in expressions like `relevant`, and + * perhaps more generally. + * + * - There are some unaddressed casting considerations which **might be** + * implied by this, discussed in greater detail in porting notes on + * {@link UntypedAnswer}. + * + * Two additional variants of this test are added immediately following this + * one, both briefly exploring some of the contours of the current failure. + */ + it.fails('is inherited from ancestors', async () => { + const scenario = await Scenario.init( + 'Some form', + html( + head( + title('Some form'), + model( + mainInstance( + t( + 'data id="some-form"', + t('is-group-relevant'), + t('is-field-relevant'), + t('group', t('field')) + ) + ), + bind('/data/is-group-relevant').type('boolean'), + bind('/data/is-field-relevant').type('boolean'), + bind('/data/group').relevant('/data/is-group-relevant'), + bind('/data/group/field').type('string').relevant('/data/is-field-relevant') + ) + ), + body( + input('/data/is-group-relevant'), + input('/data/is-field-relevant'), + group('/data/group', input('/data/group/field')) + ) + ) + ); + + // Form initialization evaluates all triggerables, which makes the group and + //field non-relevants because their relevance expressions are not satisfied + expect(scenario.getAnswerNode('/data/group')).toBeNonRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeNonRelevant(); + + // Now we make both relevant + scenario.answer('/data/is-group-relevant', true); + scenario.answer('/data/is-field-relevant', true); + + expect(scenario.getAnswerNode('/data/group')).toBeRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeRelevant(); + + // Now we make the group non-relevant, which makes the field non-relevant + // regardless of its local relevance expression, which would be satisfied + // in this case + scenario.answer('/data/is-group-relevant', false); + + expect(scenario.getAnswerNode('/data/group')).toBeNonRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeNonRelevant(); + }); + + /** + * **PORTING NOTES** (first variant of ported test above) + * + * This test is identical to the test above, except that both `relevant` + * expressions are wrapped in a `string()` XPath call. The test still fails, + * but notably the failing assertion comes later: + * + * In the original test, the first assertion fails because a `node-set` + * expression which resolves to any node will always cast to `true`. When + * the value is cast to a string, the node's text value is consulted in + * casting, producing `false` when empty. + * + * Ultimately, the test fails when checking restoration of the `false` + * state. This is because the `false` value is presently being persisted to + * the primary instance as the string `"0"` (which, as I recall, is the + * expected serialization of boolean `false`). Since the `relevant` + * expression itself produces a string value, and with the engine still + * following strict XPath casting semantics, the value `"0"` is also cast to + * boolean `true` (again, consistent with XPath semantics). + */ + it.fails('is inherited from ancestors (variant #1: node-set semantics -> string)', async () => { + const scenario = await Scenario.init( + 'Some form', + html( + head( + title('Some form'), + model( + mainInstance( + t( + 'data id="some-form"', + t('is-group-relevant'), + t('is-field-relevant'), + t('group', t('field')) + ) + ), + bind('/data/is-group-relevant').type('boolean'), + bind('/data/is-field-relevant').type('boolean'), + bind('/data/group').relevant('string(/data/is-group-relevant)'), + bind('/data/group/field').type('string').relevant('string(/data/is-field-relevant)') + ) + ), + body( + input('/data/is-group-relevant'), + input('/data/is-field-relevant'), + group('/data/group', input('/data/group/field')) + ) + ) + ); + + // Form initialization evaluates all triggerables, which makes the group and + //field non-relevants because their relevance expressions are not satisfied + expect(scenario.getAnswerNode('/data/group')).toBeNonRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeNonRelevant(); + + // Now we make both relevant + scenario.answer('/data/is-group-relevant', true); + scenario.answer('/data/is-field-relevant', true); + + expect(scenario.getAnswerNode('/data/group')).toBeRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeRelevant(); + + // Now we make the group non-relevant, which makes the field non-relevant + // regardless of its local relevance expression, which would be satisfied + // in this case + scenario.answer('/data/is-group-relevant', false); + + expect(scenario.getAnswerNode('/data/group')).toBeNonRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeNonRelevant(); + }); + + /** + * **PORTING NOTES** (second variant) + * + * This variant of the ported JavaRosa test again casts the `relevant` + * expressions, this time to `number`. Here we see the test passes! This + * variant is included because it demonstrates all of the findings above, by + * showing how strict XPath casting semantics interact with the test form's + * expected XForms semantics. + */ + it('is inherited from ancestors (variant #2: node-set semantics -> number)', async () => { + const scenario = await Scenario.init( + 'Some form', + html( + head( + title('Some form'), + model( + mainInstance( + t( + 'data id="some-form"', + t('is-group-relevant'), + t('is-field-relevant'), + t('group', t('field')) + ) + ), + bind('/data/is-group-relevant').type('boolean'), + bind('/data/is-field-relevant').type('boolean'), + bind('/data/group').relevant('number(/data/is-group-relevant)'), + bind('/data/group/field').type('string').relevant('number(/data/is-field-relevant)') + ) + ), + body( + input('/data/is-group-relevant'), + input('/data/is-field-relevant'), + group('/data/group', input('/data/group/field')) + ) + ) + ); + + // Form initialization evaluates all triggerables, which makes the group and + //field non-relevants because their relevance expressions are not satisfied + expect(scenario.getAnswerNode('/data/group')).toBeNonRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeNonRelevant(); + + // Now we make both relevant + scenario.answer('/data/is-group-relevant', true); + scenario.answer('/data/is-field-relevant', true); + + expect(scenario.getAnswerNode('/data/group')).toBeRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeRelevant(); + + // Now we make the group non-relevant, which makes the field non-relevant + // regardless of its local relevance expression, which would be satisfied + // in this case + scenario.answer('/data/is-group-relevant', false); + + expect(scenario.getAnswerNode('/data/group')).toBeNonRelevant(); + expect(scenario.getAnswerNode('/data/group/field')).toBeNonRelevant(); + }); + }); +});