Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Structurally compare js objects rather than YAML #404

Merged
merged 3 commits into from
Sep 28, 2021
Merged

Structurally compare js objects rather than YAML #404

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
604cbe6
Structurally compare js objects rather than YAML
Bj0rnen Sep 16, 2021
e69a55b
Correct the return type in the signature
Bj0rnen Sep 16, 2021
cfd7628
Correct the return type in the signature (again)
Bj0rnen Sep 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ts/kpt-functions/src/testing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import { Configs, KptFunc } from './types';
import { safeDump } from 'js-yaml';
import { safeDump, safeLoad } from 'js-yaml';

/**
* TestRunner makes it easy to write unit tests for kpt functions.
Expand Down Expand Up @@ -129,12 +129,13 @@ async function testFn(
}
}

function valueOf(configs: Configs): string {
function valueOf(configs: Configs): any {
const output = configs.toResourceList();
return safeDump(output, {
const yaml = safeDump(output, {
indent: 2,
noArrayIndent: true,
skipInvalid: true,
sortKeys: true,
});
return safeLoad(yaml);
Copy link
Contributor

@mengqiy mengqiy Sep 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to safeDump and then safeLoad?
Can we simply compare the ResourceList returned by configs.toResourceList()? Are there any downsides?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that it's a roundabout way to implement it. But in practice, returning configs.toResourceList() causes many test failures for us. Mainly these two types of inequalities:

Expected $.items[0].metadata not to have properties
        namespace: undefined

and

Expected $.items[0] to be a kind of IAMPolicyResource, but was Object({ apiVersion: 'iam.cnrm.cloud.google.com/v1alpha2', kind: 'IAMPolicy', metadata: Object({ name: 'iam-policy', namespace: 'project-level' }), spec: Object({ resourceRef: Object({ apiVersion: 'test', kind: 'Test' }), bindings: [  ] }) }).

The first kind of inequality is easy to understand. safeDump elides all undefined properties.

The second kind of inequality results from the call to deepCopy on line 102

kpt-functions-sdk/ts/kpt-functions/src/testing.ts

Line 102 in cfd7628

const configs = input.deepCopy();

I haven't dug into the implementation of Configs.deepCopy. But basically, before the equality comparison happens, the input object goes through a deepCopy and the expectedOutput object doesn't. That deepCopy is apparently not "equality-safe", however whatever differences it creates disappear again upon safeDump.

We have already relied on the semantics of post-yaml comparison in our test cases. 44/118 of our test cases break if we directly return configs.toResourceList() from valueOf. safeDump returns a string. And if strings s and t are equal (and valid YAML), then safeLoad(s) and safeLoad(t) must also be equal (safeDump is deterministic, surely). The proposed change can't break existing tests.

Alternatives

Patch the differences

I suppose it's possible to address the two types of equality differences that we have observed above. We can do a deepCopy of the o object that goes into the comparison. We can explicitly filter out undefined properties with some recursive function. Or maybe modify our kpt functions to be meticulous about this, so that all the undefineds match up.

But how can we know that these two equality differences are the only ones that could appear? Someone else might depend on other aspects of safeDump in their tests. It seems a bit arbitrary to patch over these two things specifically.

Rich YAML comparison

Maybe the expect(...).toEqual(...) can be replaced with some YAML-specific equality comparison function, which takes two YAMLs and outputs a rich diff on the strings? For example a line-based diff. Then we can go back to only using safeDump and not safeLoad. That should probably also output readable diffs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation.

}