Skip to content

Commit

Permalink
Merge pull request #135 from getodk/bugs/repeats-grab-bag
Browse files Browse the repository at this point in the history 
Several repeat-related fixes
  • Loading branch information
@eyelidlessness
eyelidlessness authored Jun 27, 2024
2 parents 95e866f + 46bf4ec commit d121679
Show file tree
Hide file tree
Showing 25 changed files with 685 additions and 606 deletions.
12 changes: 12 additions & 0 deletions .changeset/neat-spoons-pull.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"@getodk/scenario": patch
"@getodk/xforms-engine": patch
---

Several repeat-related fixes:

- Fix: most cases of inconsistent children state in Solid-based clients
- Fix: many cases of incomplete functionality on/within N > 1 repeat instances
- Fix: computation of bind states (`relevant` especially) before values, properly clear non-relevant default values
- Fix: timing inconsistency of some computations on init, adding repeat instances
- Fix: application of `relevant` state where expression is on repeat itself
39 changes: 21 additions & 18 deletions packages/scenario/test/form-definition-validity.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ describe('TriggerableDagTest.java', () => {
// exceptionRule.expectMessage("Cycle detected in form's relevant and calculation logic!");

const init = async () => {
return Scenario.init(
await Scenario.init(
'Some form',
buildFormForDagCyclesCheck(bind('/data/count').type('int').calculate('. + 1'))
);
Expand Down Expand Up @@ -164,16 +164,19 @@ describe('TriggerableDagTest.java', () => {
* environments currently produces a different error message. So for
* now, we just check that an error is produced at all.
*/
it('should fail (supplementary/alternate test with bogus error message check)', async () => {
const init = async () => {
return Scenario.init(
'Some form',
buildFormForDagCyclesCheck(bind('/data/count').type('int').calculate('. + 1'))
);
};

await expect(init).rejects.toThrow();
});
it.fails(
'should fail (supplementary/alternate test with bogus error message check)',
async () => {
const init = async () => {
await Scenario.init(
'Some form',
buildFormForDagCyclesCheck(bind('/data/count').type('int').calculate('. + 1'))
);
};

await expect(init).rejects.toThrow();
}
);
});

/**
Expand Down Expand Up @@ -255,12 +258,12 @@ describe('TriggerableDagTest.java', () => {
* parameterized ("table") test?
*/
describe('by self reference in relevance', () => {
it('should fail', async () => {
it.fails('should fail', async () => {
// exceptionRule.expect(XFormParseException.class);
// exceptionRule.expectMessage("Cycle detected in form's relevant and calculation logic!");

const init = async () => {
return Scenario.init(
await Scenario.init(
'Some form',
buildFormForDagCyclesCheck(bind('/data/count').type('int').relevant('. > 0'))
);
Expand All @@ -271,12 +274,12 @@ describe('TriggerableDagTest.java', () => {
});

describe('by self reference in `readonly` condition', () => {
it('should fail', async () => {
it.fails('should fail', async () => {
// exceptionRule.expect(XFormParseException.class);
// exceptionRule.expectMessage("Cycle detected in form's relevant and calculation logic!");

const init = async () => {
return Scenario.init(
await Scenario.init(
'Some form',
buildFormForDagCyclesCheck(bind('/data/count').type('int').readonly('. > 10'))
);
Expand All @@ -287,12 +290,12 @@ describe('TriggerableDagTest.java', () => {
});

describe('by self reference in required condition', () => {
it('should fail', async () => {
it.fails('should fail', async () => {
// exceptionRule.expect(XFormParseException.class);
// exceptionRule.expectMessage("Cycle detected in form's relevant and calculation logic!");

const init = async () => {
return Scenario.init(
await Scenario.init(
'Some form',
buildFormForDagCyclesCheck(bind('/data/count').type('int').required('. > 10'))
);
Expand Down Expand Up @@ -581,7 +584,7 @@ describe('TriggerableDagTest.java', () => {
// exceptionRule.expectMessage("Cycle detected in form's relevant and calculation logic!");

const init = async () => {
return Scenario.init(
await Scenario.init(
'Some form',
html(
head(
Expand Down
150 changes: 126 additions & 24 deletions packages/scenario/test/relevant.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,38 +410,51 @@ describe('Relevance - TriggerableDagTest.java', () => {
});

/**
* **PORTING NOTES** (supplemental to test above)
* **PORTING NOTES** (supplemental: 1 of 3)
*
* This test exercises different semantics than the test above ported from
* JavaRosa, but would also serve to exercise the concept that non-relevance
* excludes a node from evaluation: specifically by virtue of its value
* being blank.
* - This test extends the fixture above by adding a `calculate` of the
* affected nodes. This allows exercising non-relevant value exclusion
* without asserting implementation details as in the original test as
* ported from JavaRosa.
*
* Unfortunately, it also reveals a bug in the engine's relevance
* computation logic! At a glance, it appears that:
* - A previous iteration of this alternate test had a substantially
* different form fixture shape, without many of the hallmarks of the
* original test. This alternate was then updated to more closely align
* with the intent of the original. Unfortunately that caused the test's
* failure mode to diverge from details in the PORTING NOTES.
*
* 1. The `calculate` is evaluated against the nodes' default values
* 2. Before relevance is computed for those nodes
* 3. Finally, failing to recompute the `calculate` once those nodes'
* non-relevance is established
* - There are actually three failure modes **in this test, as it stands**:
*
* - - -
* 1. A bug where `relevant` is applied after the nodes' initial value
* is set, and fails to cause those nodes to be cleared when that
* relevance state is finally computed.
*
* The original iteration of this supplemental test had an entirely
* different form shape. Since we determined in review that the intent was
* to test with repeats, it was much easier to adapt the fixture to
* demonstrate the same supplemental/alternate approach.
* 2. A bug where `relevant` conditions which should affect each repeat
* instance independently, also causes the repeat *range* to be
* treated as non-relevant. This then cascades as inherited
* non-relevance, effectively making all repeat instances
* non-relevant if any of them are.
*
* - - -
* 3. An apparent discrepancy between JavaRosa and Web Forms semantics
* when `position()` is called without an argument, with the repeat
* instance as its context. As I understand the
* {@link https://getodk.github.io/xforms-spec/#fn:position | `position` spec},
* JavaRosa's behavior is roughly consistent with the 1-arity
* extension (e.g. as if the function had been called as
* `position(current())`).
*
* A very brief spike revealed that fixing this will be trivial. It also
* revealed that it's highly likely this currently affects _only_ form
* default values. As such, the current description reflects that.
* - Given these three failure modes, two **additional** supplemental tests
* are added below, eliminating consideration of (3) and then (2)
* respectively (in that order to keep the least divergent fixtures
* closest together). This test will remain intact as an exercise of all
* three failure modes. This will allow us to fix the first two bugs
* separately, and then address the discrepancy in (3) after we have a
* chance to discuss whether it's expected behavior.
*
* I believe this is probably a valuable test in its own right, given that
* understanding of the bug's likely scope, and that we may want to increase
* coverage of initial state conditions generally. **This will also probably
* be something to consider when we work on support for editing.**
* - My instinct is that JavaRosa's behavior per (3) above **is expected**,
* despite deviation from the apparent letter of the spec. This instinct
* is largely based in tracking the behavior back to
* {@link https://github.com/getodk/javarosa/blame/980a36b99680c4dccff3cb634f984ed9f93df800/src/org/javarosa/xpath/expr/XPathFuncExpr.java#L282-L286 | JavaRosa's first commit ("added trig functions" 🙃)}.
*/
it.fails(
'is excluded from producing default values in an evaluation (supplemental to two previous tests)',
Expand Down Expand Up @@ -476,6 +489,95 @@ describe('Relevance - TriggerableDagTest.java', () => {
expect(scenario.answerOf('/data/node-values')).toEqualAnswer(stringAnswer('345'));
}
);

/**
* **PORTING NOTES** (supplemental: 2 of 3; see commit history for
* additional context and commentary)
*/
it('is excluded from producing default values in an evaluation (supplemental to two previous tests)', async () => {
const scenario = await Scenario.init(
'Some form',
html(
head(
title('Some form'),
model(
mainInstance(
t(
'data id="some-form"',
// position() is one-based
t('node', t('value', '1')), // non-relevant
t('node', t('value', '2')), // non-relevant
t('node', t('value', '3')), // relevant
t('node', t('value', '4')), // relevant
t('node', t('value', '5')), // relevant
t('node-values')
)
),
bind('/data/node').relevant('position(current()) > 2'),
bind('/data/node/value').type('int'),
bind('/data/node-values').calculate('concat(/data/node/value)')
)
),
body(group('/data/node', repeat('/data/node', input('/data/node/value'))))
)
);

expect(scenario.answerOf('/data/node-values')).toEqualAnswer(stringAnswer('345'));
});

/**
* **PORTING NOTES** (supplemental: 3 of 3; see commit history for
* additional context and commentary)
*/
it('excludes default values on nodes which are non-relevant on init', async () => {
const scenario = await Scenario.init(
'Some form',
html(
head(
title('exclusion of non-relevant default values'),
model(
mainInstance(
t(
'data id="exclusion-of-non-relevant-default-values"',
t('is-node-a-relevant', 'no'),
t('is-node-b-relevant', 'no'),
t('is-node-c-relevant', 'yes'),
t('is-node-d-relevant', 'yes'),
t('is-node-e-relevant', 'yes'),
// position() is one-based
t('node-a', t('value', '1')), // non-relevant
t('node-b', t('value', '2')), // non-relevant
t('node-c', t('value', '3')), // relevant
t('node-d', t('value', '4')), // relevant
t('node-e', t('value', '5')), // relevant,
t('node-x-concat') // calculates a concatenation of node-a through node-e
)
),
bind('/data/node-a').relevant("/data/is-node-a-relevant = 'yes'"),
bind('/data/node-b').relevant("/data/is-node-b-relevant = 'yes'"),
bind('/data/node-c').relevant("/data/is-node-c-relevant = 'yes'"),
bind('/data/node-d').relevant("/data/is-node-d-relevant = 'yes'"),
bind('/data/node-e').relevant("/data/is-node-e-relevant = 'yes'"),
bind('/data/node-x-concat').calculate(
'concat(/data/node-a, /data/node-b, /data/node-c, /data/node-d, /data/node-e)'
),
bind('/data/node/value').type('int')
)
),

body(
group('/data/node-a', input('/data/node-a/value')),
group('/data/node-b', input('/data/node-b/value')),
group('/data/node-c', input('/data/node-c/value')),
group('/data/node-d', input('/data/node-d/value')),
group('/data/node-e', input('/data/node-e/value')),
input('/data/node-x-concat')
)
)
);

expect(scenario.answerOf('/data/node-x-concat')).toEqualAnswer(stringAnswer('345'));
});
});

describe('non-relevant node values', () => {
Expand Down
68 changes: 11 additions & 57 deletions packages/scenario/test/repeat-relevant.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,64 +394,18 @@ describe('Interaction between `<repeat>` and `relevant`', () => {
/**
* **PORTING NOTES**
*
* 1. Liberty was taken to omit a position predicate on a non-repeat node
* in one of the ported assertions. While JavaRosa has a notion of
* normalizing expressions (e.g. comparing
* `/data/repeat[1]/non-repeat[1]` as equivalent to
* `/data/repeat[1]/non-repeat`), the web forms engine interface does
* not expose any such notion. This doesn't seem like an issue that
* most clients should be concerned with, so it seems most reasonable
* to port the assertion without requiring such a normalization. If
* that assumption turns out to be wrong, we can revisit in the future.
* The orignal assertion is commented directly above.
*
* 2. Failure anlaysis in some depth follows...
*
* - - -
*
* Fails with observed behavior that the second repeat instance's
* descendants never become relevant. As such, only the assertions before
* the value change fail.
*
* To keep the porting effort's momentum, I've generally tried to defer
* investigating any failure more deeply than gaining an understanding of
* the test case itself and hypothesizing the nature of the failure based
* on my understanding of engine behavior. However, this test was
* confusing enough (on first read) and its behavior surprising enough
* that I did a quick spike. Here's what I found (and some commentary):
*
* - The apparent root cause is that we are incorrectly evaluating repeat
* instance `relevant` expressions in the context of both the repeat
* instance itself (as expected) and its parent element (by mistake).
*
* - In a sense, this is a straightforward bug. It involves a clear (in
* hindsight) logical error. It can be fixed with a relatively
* straightforward special case.
*
* - In another sense, this is a symptom of a design flaw. There are
* likely several very similar issues lurking with essentially the same
* logical error (with at least one other which is clearly present in
* exactly the same method, just for another bind expression).
* Fundamentally, this bug (and others like it) are able to happen
* because of the impedance mismatch between the engine's structural
* model (wherein a "repeat range" is a thing that exists, and is
* treated as a node with the same hierarchical qualities as any other)
* and the engine's use of browser/XML DOM as an implementation of that.
*
* - This is an excellent example of the correctness implications of that
* latter implementation detail, and a good one to consider as we think
* about prioritization of an effort to move away from it.
*
* Note that even addressing the bug found above, this test would fail for
* another reason: we don't currently have a notion of determining
* relevance of a "repeat range" (and thus whether it should present a
* "prompt" event in JavaRosa/Scenario terms) by the relevance of its
* instances. There are other tests addressing this concern more directly,
* so it's likely we'll be able to target that issue with those. This is
* mostly meant as an observation that the two concerns intersect in this
* test even though it's not explicit.
* Liberty was taken to omit a position predicate on a non-repeat node
* in one of the ported assertions. While JavaRosa has a notion of
* normalizing expressions (e.g. comparing
* `/data/repeat[1]/non-repeat[1]` as equivalent to
* `/data/repeat[1]/non-repeat`), the web forms engine interface does
* not expose any such notion. This doesn't seem like an issue that
* most clients should be concerned with, so it seems most reasonable
* to port the assertion without requiring such a normalization. If
* that assumption turns out to be wrong, we can revisit in the future.
* The orignal assertion is commented directly above.
*/
it.fails('updates based on parent position', async () => {
it('updates based on parent position', async () => {
const scenario = await Scenario.init(
'Nested repeat relevance',
html(
Expand Down
Loading

0 comments on commit d121679

Please sign in to comment.