TriggerableDagTest.java - port first relevant test…

… with extensive porting notes mostly about casting semantics, and a couple of test variants demonstrating some of the factors at play in the original test/detailed in those notes.

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 `<bind type>`
+ * 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 `<bind type>` **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** `<bind type>`-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) {

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...

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 `<bind
+ * type="boolean" />`, 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();
+ });
+ });
+});