Conditional rendering testing value of object of forward/backward link

[jg10-mastodon-social]

Closes #184

• Add some-value-eq and every-value-eq
• Add Thing.observeLiterals
• Support literals for if-property
• Add (some|every)-value-(gt|gte|lt|lte)
• Tests have been written
  • all new code is covered by unit tests
  • the happy path of a new feature is covered by an end-to-end test
    • added to pos-switch integration test
  • manual explorative tests have been performed
• all dependencies are updated to the latest patch version at minimum
• the CI pipeline passes
• documentation is up-to-date
  • TSDoc style comments on important functions, properties and events
  • stories for new PodOS elements have been added to storybook
  • Readme.md files of PodOS elements have been re-generated
  • N/A- architectural decisions are documented as an ADR
  • Changelogs are updated according to Keep a Changelog

[jg10-mastodon-social]

I suspect we might need more tests here, but I'm not sure how to design them.
I designed these as TDD tests - they all fail if the numbers are treated as strings.

[jg10-mastodon-social]

I've now grouped tests, and split out tests specifically of the test method.
I've tried to systematically go through possible combinations of inputs and data, using the it.each approach.

The code is still missing systematic tests of:

• evaluating values of property and rev with multiple relations
• evaluating values of property with multiple strings
• evaluating values of property with multiple numbers

There are ad-hoc tests for this functionality, so given the difficulty in parameterising these combinatorial tests, I suggest we open an issue documenting this gap and merge in the mean time.

Any suggestions are welcome for systematically testing all combinations in the presence of multiple values!

[angelo-v]

@jg10-mastodon-social Sorry, I just noticed I left some comments in "pending" state and you propbably did not see them. I am just submitting them now - yet I did not find the time to look at your latest changes, so I am not sure if they are still relevant. I am going to continue reviewing the current state later this week

[angelo-v]

issue: this function is very complex and should a) be refactored into smaller chunks and b) moved into its own file and heavily unit tested. Comparision logic be separated from element attributes.

[jg10-mastodon-social]

I hadn't seen this comment and have therefore not refactored. Any help in dividing it up would be great!

I did however create a separate test file for this function so (b) is partially addressed.

I'm not sure about separating out comparison logic from element attributes - the main purpose of the function is to apply all necessary combinations of element attributes - the comparison logic itself is arguably trivial once the effect of the element attributes is applied.

[angelo-v]

my fault! I did not submit it properly and am very sorry. I added a refactoring commit that goes into the direction I tried to explain, but it is still a long way to go. I am sorry to say that but in the current state the complexity in pos-switch is unacceptable to me and propably still a source of many bugs.

Thank you very much for the extensive test suite you added. These tests are a necessary security net for refactoring. Yet the combinatory explosion of these tests and the fact that they are yet not able to cover every case are a big big smell.

pos-switch does effectifly three things:

1. data gathering: find all the necessary data for comparision (observed relations, values etc)
2. rules: gather semantics, operators and their respective values applied to pos-case (like "some-value-eg=..." etc.)
3. apply the rules to the gathered data

I imagine that those 3 parts could be well separated and tested by themselves and this would reduce the testing combinatoric a lot

[angelo-v]

I am thinking of something like that:

const data = {
  literals: [],
  relations: [],
  reverseRelations: [],
  types: []
};

const rule = {
  type: "if-property",
  value: "https://schema.org/name",
  operator: "eq",
  semantic: "some",
  target: "Alice"
}

function ruleMatches(r: typeof rule, d: typeof data): boolean {
}

1. collect data from resource
2. create rule(s) from dom attributes
3. match rules with data

ruleMatches is a pure function with no dependency on dom or resource that could be easily testable. Also there can be multiple sub-functions for different rule types that can be tested and focus on that specific rule

[angelo-v]

There is Element.getAttributeNames that might make it easier to get the attributes from dom compared to checking if each exists individually, but unfortunately stencil testbed does not implement it. This should not stop us from using it, if it helps us. If the pure function based on configured rules does the heavy logic we can easily mock getAttributeNames for the component testing. I am also currently migrating to vitest and this might make the stencil testing situation better.

[jg10-mastodon-social]

I think if-revrules would be processed separately.

const rule = {
  type: "if-rev",
  value: "https://schema.org/item",
  operator: "eq",
  semantic: "some",
  target: "https://resource"
}

// If rule.type="if-rev"
const matchingRelations = data.reverseRelations.filter(x => x.predicate == rule.value);
if (matchingRelations.length > 0) {
  dataValues = matchingRelations[0].uris;
}

state = state && testIfValuesMatchTarget(
  dataValues,
  rule.semantics,
  rule.operator,
  rule.target,
)

[jg10-mastodon-social]

This covers most of

PodOS/elements/src/components/pos-switch/pos-switch.test.spec.tsx

Lines 483 to 850 in 7417242

	describe('evaluating values of property and rev (single relations)', () => {
	/*
	80 possible combinations:
	- Predicates: if-property, if-rev
	- Condition: (some|every)-(eq|lt|lte|gt|gte)
	- Modifier: negation
	- Evaluation state: matched, not matched
	We test three resource values (120 combinations) matching eq, lt, or gt
	There is no difference between some and every with a single relation.
	*/
	let testCases = [];
	for (let direction of ['if-property', 'if-rev']) {
	for (let semantics of ['some', 'every']) {
	for (let operator of ['eq', 'gt', 'gte', 'lt', 'lte']) {
	for (let negation of [true, false]) {
	for (let resource of [
	'https://resource.test/resource-1',
	'https://resource.test/resource-2',
	'https://resource.test/resource-3',
	]) {
	const cmp = resource.localeCompare('https://resource.test/resource-2');
	let matches;
	switch (operator) {
	case 'eq':
	matches = cmp === 0;
	break;
	case 'gt':
	matches = cmp > 0;
	break;
	case 'gte':
	matches = cmp >= 0;
	break;
	case 'lt':
	matches = cmp < 0;
	break;
	case 'lte':
	matches = cmp <= 0;
	break;
	}
	matches = negation ? !matches : matches;
	const result = matches ? 'matched' : 'not matched';
	const not = negation ? 'not' : '';
	testCases.push({
	direction: direction,
	conditions: `${direction}="https://schema.org/video" ${not} ${semantics}-value-${operator}="https://resource.test/resource-2"`,
	resource: resource,
	expectedResult: result,
	});
	}
	}
	}
	}
	}
	expect(testCases.filter(testCase => testCase.expectedResult == 'matched').length).toEqual(60);
	expect(testCases.filter(testCase => testCase.expectedResult == 'not matched').length).toEqual(60);
	it.each(testCases)(
	`renders templates if condition is met: $conditions for $resource `,
	async ({ direction, conditions, resource, expectedResult }) => {
	const page = await newSpecPage({
	components: [PosSwitch],
	html: `
	<pos-switch>
	<pos-case ${conditions}>
	<template>
	<div>Condition is matched</div>
	</template>
	</pos-case>
	<pos-case else>
	<template>
	<div>Condition is not matched</div>
	</template>
	</pos-case>
	</pos-switch>`,
	});
	const observedRelations$ = new Subject<Relation[]>();
	const observedLiterals$ = new Subject<Literal[]>();
	const observedReverseRelations$ = new Subject<Relation[]>();
	const thing = {
	uri: 'https://pod.example/resource',
	observeRelations: () => observedRelations$,
	observeLiterals: () => observedLiterals$,
	observeReverseRelations: () => observedReverseRelations$,
	};
	const relations = [
	{
	predicate: 'https://schema.org/video',
	label: 'video',
	uris: [resource],
	},
	];
	page.rootInstance.receiveResource(thing);
	observedLiterals$.next([]);
	if (direction == 'if-property') {
	observedRelations$.next(relations);
	observedReverseRelations$.next([]);
	} else {
	observedRelations$.next([]);
	observedReverseRelations$.next(relations);
	}
	await page.waitForChanges();
	expect(page.root?.innerHTML).toEqualHtml(`
	<div>Condition is ${expectedResult}</div>
	`);
	},
	);
	});
	describe('evaluating values of property and rev (no relations or literals)', () => {
	/*
	40 possible combinations:
	- Predicates: if-property, if-rev
	- Condition: (some|every)-(eq|lt|lte|gt|gte)
	- Modifier: negation
	- Evaluation state: matched, not matched
	Should always return not matched, or matched with negation
	For if-property covers both cases where there are no relations or no literals
	*/
	let testCases = [];
	for (let direction of ['if-property', 'if-rev']) {
	for (let semantics of ['some', 'every']) {
	for (let operator of ['eq', 'gt', 'gte', 'lt', 'lte']) {
	for (let negation of [true, false]) {
	const result = negation ? 'matched' : 'not matched';
	const not = negation ? 'not' : '';
	testCases.push({
	direction: direction,
	conditions: `${direction}="https://schema.org/video" ${not} ${semantics}-value-${operator}="https://resource.test/resource-2"`,
	expectedResult: result,
	});
	}
	}
	}
	}
	expect(testCases.filter(testCase => testCase.expectedResult == 'matched').length).toEqual(20);
	expect(testCases.filter(testCase => testCase.expectedResult == 'not matched').length).toEqual(20);
	it.each(testCases)(
	`renders templates if condition is met: $conditions for no relations `,
	async ({ conditions, expectedResult }) => {
	const page = await newSpecPage({
	components: [PosSwitch],
	html: `
	<pos-switch>
	<pos-case ${conditions}>
	<template>
	<div>Condition is matched</div>
	</template>
	</pos-case>
	<pos-case else>
	<template>
	<div>Condition is not matched</div>
	</template>
	</pos-case>
	</pos-switch>`,
	});
	const observedRelations$ = new Subject<Relation[]>();
	const observedLiterals$ = new Subject<Literal[]>();
	const observedReverseRelations$ = new Subject<Relation[]>();
	const thing = {
	uri: 'https://pod.example/resource',
	observeRelations: () => observedRelations$,
	observeLiterals: () => observedLiterals$,
	observeReverseRelations: () => observedReverseRelations$,
	};
	page.rootInstance.receiveResource(thing);
	observedLiterals$.next([]);
	observedRelations$.next([]);
	observedReverseRelations$.next([]);
	await page.waitForChanges();
	expect(page.root?.innerHTML).toEqualHtml(`
	<div>Condition is ${expectedResult}</div>
	`);
	},
	);
	});
	// TODO evaluating values of property and rev with multiple relations
	describe('evaluating values of property (single strings)', () => {
	/*
	40 possible combinations:
	- Condition: (some|every)-(eq|lt|lte|gt|gte)
	- Modifier: negation
	- Evaluation state: matched, not matched
	We test three resource values (60 combinations) matching eq, lt, or gt
	There is no difference between some and every with a single relation.
	*/
	let testCases = [];
	for (let semantics of ['some', 'every']) {
	for (let operator of ['eq', 'gt', 'gte', 'lt', 'lte']) {
	for (let negation of [true, false]) {
	for (let stringValue of ['a', 'b', 'c']) {
	const cmp = stringValue.localeCompare('b');
	let matches;
	switch (operator) {
	case 'eq':
	matches = cmp === 0;
	break;
	case 'gt':
	matches = cmp > 0;
	break;
	case 'gte':
	matches = cmp >= 0;
	break;
	case 'lt':
	matches = cmp < 0;
	break;
	case 'lte':
	matches = cmp <= 0;
	break;
	}
	matches = negation ? !matches : matches;
	const result = matches ? 'matched' : 'not matched';
	const not = negation ? 'not' : '';
	testCases.push({
	conditions: `if-property="https://schema.org/name" ${not} ${semantics}-value-${operator}="b"`,
	stringValue: stringValue,
	expectedResult: result,
	});
	}
	}
	}
	}
	expect(testCases.filter(testCase => testCase.expectedResult == 'matched').length).toEqual(30);
	expect(testCases.filter(testCase => testCase.expectedResult == 'not matched').length).toEqual(30);
	it.each(testCases)(
	`renders templates if condition is met: $conditions for $resource `,
	async ({ conditions, stringValue, expectedResult }) => {
	const page = await newSpecPage({
	components: [PosSwitch],
	html: `
	<pos-switch>
	<pos-case ${conditions}>
	<template>
	<div>Condition is matched</div>
	</template>
	</pos-case>
	<pos-case else>
	<template>
	<div>Condition is not matched</div>
	</template>
	</pos-case>
	</pos-switch>`,
	});
	const observedRelations$ = new Subject<Relation[]>();
	const observedLiterals$ = new Subject<Literal[]>();
	const observedReverseRelations$ = new Subject<Relation[]>();
	const thing = {
	uri: 'https://pod.example/resource',
	observeRelations: () => observedRelations$,
	observeLiterals: () => observedLiterals$,
	observeReverseRelations: () => observedReverseRelations$,
	};
	page.rootInstance.receiveResource(thing);
	observedLiterals$.next([{ predicate: 'https://schema.org/name', label: 'name', values: [stringValue] }]);
	observedRelations$.next([]);
	observedReverseRelations$.next([]);
	await page.waitForChanges();
	expect(page.root?.innerHTML).toEqualHtml(`
	<div>Condition is ${expectedResult}</div>
	`);
	},
	);
	});
	// TODO evaluating values of property with multiple strings
	describe('evaluating values of property (single number)', () => {
	/*
	40 possible combinations:
	- Condition: (some|every)-(eq|lt|lte|gt|gte)
	- Modifier: negation
	- Evaluation state: matched, not matched
	We test three values (60 combinations) matching eq, lt, or gt
	The values are selected so that string comparison is insufficient.
	There is no difference between some and every with a single number.
	*/
	let testCases = [];
	for (let semantics of ['some', 'every']) {
	for (let operator of ['eq', 'gt', 'gte', 'lt', 'lte']) {
	for (let negation of [true, false]) {
	for (let stringValue of ['3', '20', '100']) {
	const cmp = Number(stringValue) - 20;
	let matches;
	switch (operator) {
	case 'eq':
	matches = cmp === 0;
	break;
	case 'gt':
	matches = cmp > 0;
	break;
	case 'gte':
	matches = cmp >= 0;
	break;
	case 'lt':
	matches = cmp < 0;
	break;
	case 'lte':
	matches = cmp <= 0;
	break;
	}
	matches = negation ? !matches : matches;
	const result = matches ? 'matched' : 'not matched';
	const not = negation ? 'not' : '';
	testCases.push({
	conditions: `if-property="https://schema.org/value" ${not} ${semantics}-value-${operator}="20"`,
	stringValue: stringValue,
	expectedResult: result,
	});
	}
	}
	}
	}
	expect(testCases.filter(testCase => testCase.expectedResult == 'matched').length).toEqual(30);
	expect(testCases.filter(testCase => testCase.expectedResult == 'not matched').length).toEqual(30);
	it.each(testCases)(
	`renders templates if condition is met: $conditions for $resource `,
	async ({ conditions, stringValue, expectedResult }) => {
	const page = await newSpecPage({
	components: [PosSwitch],
	html: `
	<pos-switch>
	<pos-case ${conditions}>
	<template>
	<div>Condition is matched</div>
	</template>
	</pos-case>
	<pos-case else>
	<template>
	<div>Condition is not matched</div>
	</template>
	</pos-case>
	</pos-switch>`,
	});
	const observedRelations$ = new Subject<Relation[]>();
	const observedLiterals$ = new Subject<Literal[]>();
	const observedReverseRelations$ = new Subject<Relation[]>();
	const thing = {
	uri: 'https://pod.example/resource',
	observeRelations: () => observedRelations$,
	observeLiterals: () => observedLiterals$,
	observeReverseRelations: () => observedReverseRelations$,
	};
	page.rootInstance.receiveResource(thing);
	observedLiterals$.next([{ predicate: 'https://schema.org/value', label: 'value', values: [stringValue] }]);
	observedRelations$.next([]);
	observedReverseRelations$.next([]);
	await page.waitForChanges();
	expect(page.root?.innerHTML).toEqualHtml(`
	<div>Condition is ${expectedResult}</div>
	`);
	},
	);
	});
	// TODO evaluating values of property with multiple numbers

but I think combination with not would be split out and perhaps if-property and if-rev get their own tests.

[jg10-mastodon-social]

Similarly to the tests, the case with no value comparison (implemented in #183) has a simpler rule

const rule = {
  type: "if-property",
  value: "https://schema.org/name",
}

// If rule.type="if-property"
const matchingRelations = data.relations.filter(x => x.predicate == rule.value);
const matchingLiterals = data.literals.filter(x => x.predicate == rule.value);

state = matchingRelations.length > 0 || matchingLiterals.length > 0;

Tests are those in

PodOS/elements/src/components/pos-switch/pos-switch.test.spec.tsx

Line 136 in 7417242

describe('if-property presence/absence', () => {

again presumably without not.

Same pattern for

PodOS/elements/src/components/pos-switch/pos-switch.test.spec.tsx

Line 360 in 7417242

describe('if-rev presence/absence', () => {

[jg10-mastodon-social]

For

PodOS/elements/src/components/pos-switch/pos-switch.test.spec.tsx

Line 17 in 7417242

describe('if-typeof', () => {

const rule = {
  type: "if-typeof",
  value: "https://schema.org/Recipe",
}

state = data.types.map(x => x.uri).includes(rule.value);

[jg10-mastodon-social]

The gathering of data is trivial because they are all already properties of the component

data = {
  literals: this.literals,
  relations: this.relations,
  reverseRelations: this.reverseRelations,
  types: this.types
}

I think they would be kept as properties of the component because we treat them as Stenciljs state variables to trigger rerender.

The collection of rule(s) from dom attributes could be by rule type, similar to the current case.

e.g.

if (caseElement.getAttribute('if-typeof') !== null) {
      rule = {
        type="if-typeof",
        value = caseElement.getAttribute('if-typeof')
      }
    }

For value comparison (focus of this PR) it's not clear to me whether iterating through operatorSemanticCombinations is still sufficient or whether parsing attribute names would be more efficient (I have assumed to date that hasAttribute is preferable to string operations).

It seems like the refactor as I've described it here would go a long way to associating the new tests with encapsulated functions.

Questions:

• Would it still be ok to evaluate the rules one at a time, similarly to how they are now? Or should they be batched?
  • The disadvantage of batching is that the rule routing would be performed twice, once for rule creation and once for rule execution.
• How would not be tested? Presumably this would require batching rules?
• Would the current tests be kept, presumably as integration tests, or would they be replaced?

[angelo-v]

issue: same as above. Try to collect the relevant data and condition from component first, then do the logic in a dedicated function that is independent from the component and well tested