Skip to content

Commit

Permalink
Roll back boolean casting fix
Browse files Browse the repository at this point in the history 
As we discussed, this can be handled in a separate scope of work. The original commits are left intact for reference, we can rebase before merge if preferred.
  • Loading branch information
@eyelidlessness
eyelidlessness committed Jul 9, 2024
1 parent 1478cae commit 3fb493e
Show file tree
Hide file tree
Showing 4 changed files with 215 additions and 191 deletions.
150 changes: 99 additions & 51 deletions packages/scenario/test/readonly.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,65 +14,113 @@ import { describe, expect, it } from 'vitest';
import { Scenario } from '../src/jr/Scenario.ts';

describe('TriggerableDagTest.java', () => {
it('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-outer-readonly'),
t('is-inner-readonly'),
t('is-field-readonly'),
t('outer', t('inner', t('field')))
interface CastReadonlyExpressionOptions {
readonly castReadonlyExpressionsAsNumber: boolean;
}

/**
* **PORTING NOTES**
*
* This test has the same semantic considerations (XPath node-set -> boolean
* casting, potential considerations for value casting on writes, etc) as the
* ported relevance test of a similar name/approach.
*
* The test has been parameterized to demonstrate this concisely, with the
* second run wrapping the `readonly` expressions in a `number()` cast (which
* passes as expected).
*
* JR:
*
* Read-only is inherited from ancestor nodes, as per the W3C XForms specs:
* - https://www.w3.org/TR/xforms11/#model-prop-relevant
*/
describe.each<CastReadonlyExpressionOptions>([
{ castReadonlyExpressionsAsNumber: false },
{ castReadonlyExpressionsAsNumber: true },
])(
'readonly (cast readonly expression as number: $castReadonlyExpressionAsNumber)',
({ castReadonlyExpressionsAsNumber }) => {
let testFn: typeof it | typeof it.fails;

if (castReadonlyExpressionsAsNumber) {
testFn = it;
} else {
testFn = it.fails;
}

let castReadonlyExpression: (baseExpression: string) => string;

if (castReadonlyExpressionsAsNumber) {
castReadonlyExpression = (baseExpression) => `number(${baseExpression})`;
} else {
castReadonlyExpression = (baseExpression) => baseExpression;
}

testFn('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-outer-readonly'),
t('is-inner-readonly'),
t('is-field-readonly'),
t('outer', t('inner', t('field')))
)
),
bind('/data/is-outer-readonly').type('boolean'),
bind('/data/is-inner-readonly').type('boolean'),
bind('/data/is-field-readonly').type('boolean'),
bind('/data/outer').readonly(castReadonlyExpression('/data/is-outer-readonly')),
bind('/data/outer/inner').readonly(
castReadonlyExpression('/data/is-inner-readonly')
),
bind('/data/outer/inner/field')
.type('string')
.readonly(castReadonlyExpression('/data/is-field-readonly'))
)
),
bind('/data/is-outer-readonly').type('boolean'),
bind('/data/is-inner-readonly').type('boolean'),
bind('/data/is-field-readonly').type('boolean'),
bind('/data/outer').readonly('/data/is-outer-readonly'),
bind('/data/outer/inner').readonly('/data/is-inner-readonly'),
bind('/data/outer/inner/field').type('string').readonly('/data/is-field-readonly')
body(
input('/data/is-outer-readonly'),
input('/data/is-inner-readonly'),
input('/data/is-field-readonly'),
group('/data/outer', group('/data/outer/inner', input('/data/outer/inner/field')))
)
)
),
body(
input('/data/is-outer-readonly'),
input('/data/is-inner-readonly'),
input('/data/is-field-readonly'),
group('/data/outer', group('/data/outer/inner', input('/data/outer/inner/field')))
)
)
);
);

// Form initialization evaluates all triggerables, which makes the field editable (not read-only)
expect(scenario.getInstanceNode('/data/outer')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeEnabled();
// Form initialization evaluates all triggerables, which makes the field editable (not read-only)
expect(scenario.getInstanceNode('/data/outer')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeEnabled();

// Make the outer group read-only
scenario.answer('/data/is-outer-readonly', true);
// Make the outer group read-only
scenario.answer('/data/is-outer-readonly', true);

expect(scenario.getInstanceNode('/data/outer')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly();

// Make the inner group read-only
scenario.answer('/data/is-outer-readonly', false);
scenario.answer('/data/is-inner-readonly', true);
// Make the inner group read-only
scenario.answer('/data/is-outer-readonly', false);
scenario.answer('/data/is-inner-readonly', true);

expect(scenario.getInstanceNode('/data/outer')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeReadonly();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly();

// Make the field read-only
scenario.answer('/data/is-inner-readonly', false);
scenario.answer('/data/is-field-readonly', true);
// Make the field read-only
scenario.answer('/data/is-inner-readonly', false);
scenario.answer('/data/is-field-readonly', true);

expect(scenario.getInstanceNode('/data/outer')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly();
});
expect(scenario.getInstanceNode('/data/outer')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner')).toBeEnabled();
expect(scenario.getInstanceNode('/data/outer/inner/field')).toBeReadonly();
});
}
);
});
52 changes: 41 additions & 11 deletions packages/scenario/test/relevant.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
import { describe, expect, it } from 'vitest';
import { intAnswer } from '../src/answer/ExpectedIntAnswer.ts';
import { stringAnswer } from '../src/answer/ExpectedStringAnswer.ts';
import type { UntypedAnswer } from '../src/answer/UntypedAnswer.ts';
import { Scenario, getRef } from '../src/jr/Scenario.ts';
import { XPathPathExpr } from '../src/jr/xpath/XPathPathExpr.ts';
import { XPathPathExprEval } from '../src/jr/xpath/XPathPathExprEval.ts';
Expand All @@ -28,7 +29,26 @@ describe('Relevance - TriggerableDagTest.java', () => {
*/

describe('non-relevance', () => {
it('is inherited from ancestors', async () => {
/**
* **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(
Expand Down Expand Up @@ -82,13 +102,23 @@ describe('Relevance - TriggerableDagTest.java', () => {
* **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. This test was
* originally introduced to demonstrate different behavior of boolean
* casting logic depending on the expression's result type. This distinction
* no longer exists, but the alternate test is preserved to help us catch
* potential regressions in boolean casting logic.
* 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('is inherited from ancestors (variant #1: node-set semantics -> string)', async () => {
it.fails('is inherited from ancestors (variant #1: node-set semantics -> string)', async () => {
const scenario = await Scenario.init(
'Some form',
html(
Expand Down Expand Up @@ -142,10 +172,10 @@ describe('Relevance - TriggerableDagTest.java', () => {
* **PORTING NOTES** (second variant)
*
* This variant of the ported JavaRosa test again casts the `relevant`
* expressions, this time to `number`. Like the previous variant, this test
* was originally introduced to demonstrate differences in casting behavior
* depending on the expression's result type. It is likewise preserved to
* help us avoid casting-related regressions in the future.
* 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(
Expand Down
114 changes: 71 additions & 43 deletions packages/scenario/test/validity-state.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,50 +86,78 @@ describe('TriggerableDagTest.java', () => {
});

describe('constraint violations and form finalization', () => {
it('[has no clear BDD-ish description equivalent]', async () => {
const scenario = await Scenario.init(
'Some form',
html(
head(
title('Some form'),
model(
mainInstance(t('data id="some-form"', t('a'), t('b'))),
bind('/data/a').type('string').constraint('/data/b'),
bind('/data/b').type('boolean')
)
),
body(input('/data/a'), input('/data/b'))
)
);

// First, ensure we will be able to commit an answer in /data/a by
// making it match its constraint. No values can be committed to the
// instance if constraints aren't satisfied.
scenario.answer('/data/b', true);

// Then, commit an answer (answers with empty values are always valid)
scenario.answer('/data/a', 'cocotero');

// Then, make the constraint defined at /data/a impossible to satisfy
scenario.answer('/data/b', false);

// At this point, the form has /data/a filled with an answer that's
// invalid according to its constraint expression, but we can't be
// aware of that, unless we validate the whole form.
//
// Clients like Collect will validate the whole form before marking
// a submission as complete and saving it to the filesystem.
//
// FormDef.validate(boolean) will go through all the relevant fields
// re-answering them with their current values in order to detect
// any constraint violations. When this happens, a non-null
// ValidationOutcome object is returned including information about
// the violated constraint.
const validate = scenario.getValidationOutcome();
interface CastReadonlyExpressionOptions {
readonly castReadonlyExpressionsAsNumber: boolean;
}

expect(validate.failedPrompt).toBe(scenario.indexOf('/data/a'));
expect(validate.outcome).toBe(ANSWER_CONSTRAINT_VIOLATED);
});
describe.each<CastReadonlyExpressionOptions>([
{ castReadonlyExpressionsAsNumber: false },
{ castReadonlyExpressionsAsNumber: true },
])(
'readonly (cast readonly expression as number: $castReadonlyExpressionAsNumber)',
({ castReadonlyExpressionsAsNumber }) => {
let testFn: typeof it | typeof it.fails;

if (castReadonlyExpressionsAsNumber) {
testFn = it;
} else {
testFn = it.fails;
}

let castReadonlyExpression: (baseExpression: string) => string;

if (castReadonlyExpressionsAsNumber) {
castReadonlyExpression = (baseExpression) => `number(${baseExpression})`;
} else {
castReadonlyExpression = (baseExpression) => baseExpression;
}

testFn('[has no clear BDD-ish description equivalent]', async () => {
const scenario = await Scenario.init(
'Some form',
html(
head(
title('Some form'),
model(
mainInstance(t('data id="some-form"', t('a'), t('b'))),
bind('/data/a').type('string').constraint(castReadonlyExpression('/data/b')),
bind('/data/b').type('boolean')
)
),
body(input('/data/a'), input('/data/b'))
)
);

// First, ensure we will be able to commit an answer in /data/a by
// making it match its constraint. No values can be committed to the
// instance if constraints aren't satisfied.
scenario.answer('/data/b', true);

// Then, commit an answer (answers with empty values are always valid)
scenario.answer('/data/a', 'cocotero');

// Then, make the constraint defined at /data/a impossible to satisfy
scenario.answer('/data/b', false);

// At this point, the form has /data/a filled with an answer that's
// invalid according to its constraint expression, but we can't be
// aware of that, unless we validate the whole form.
//
// Clients like Collect will validate the whole form before marking
// a submission as complete and saving it to the filesystem.
//
// FormDef.validate(boolean) will go through all the relevant fields
// re-answering them with their current values in order to detect
// any constraint violations. When this happens, a non-null
// ValidationOutcome object is returned including information about
// the violated constraint.
const validate = scenario.getValidationOutcome();

expect(validate.failedPrompt).toBe(scenario.indexOf('/data/a'));
expect(validate.outcome).toBe(ANSWER_CONSTRAINT_VIOLATED);
});
}
);
});
});
});
Expand Down
Loading

0 comments on commit 3fb493e

Please sign in to comment.