Skip to content

Add unification assertion mode #23

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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nkvoll
Copy link

@nkvoll nkvoll commented May 22, 2025

Adds a way of asserting via unification instead of diffing in tests.

Diffing always requires including the full desired output in the test, which for some use-cases may obfuscate the point the test is trying to show.

Unification allows to assert on just a sub-set of the complete object and leverages Cue itself to validate that the actual matches the expected output. This may of course lead to a scenario that not all of the output has been validated in each individual test, but makes it easier to see the effects the test-case is stressing.

@nkvoll nkvoll force-pushed the add-unification-assertion-mode branch from 358ad76 to f63d1d3 Compare May 22, 2025 17:23
@gotwarlost
Copy link
Collaborator

gotwarlost commented May 22, 2025

Thanks for the PR @nkvoll - after our brief conversation, I thought some more about this.

I think if you squint hard, "diff mode" is really a "unification mode" for the entire response instead of just a cherry-picked subset.

I wondered if there would be a way to get rid of the modes and have the test do the right thing in all cases, printing diffs when unification fails. It's just a kernel of an idea at this point but maybe we can flesh out how this would work?

One problem in unification mode is that if I add a random resource in the response, it will successfully unify right? I'm thinking of a typo where the test says response: desired: foo3 : resource: {} where the actual response should have been foo2. In this case since foo3 is an "extra" resource that can be unified with the response it will appear as though the test passed when it should have failed.

That is, even in unification mode we need to guard against misspelt resources and ensure that the resource name actually exists in the cue output response. So it all feels like we should be having a single way to write assertions that work correctly with partial as well as full responses.

@nkvoll
Copy link
Author

nkvoll commented May 22, 2025

One problem in unification mode is that if I add a random resource in the response, it will successfully unify right? I'm thinking of a typo where the test says response: desired: foo3 : resource: {} where the actual response should have been foo2. In this case since foo3 is an "extra" resource that can be unified with the response it will appear as though the test passed when it should have failed.

You'd get some error like this (tried): compile cue code: unified.desired.resources.foo3: field not allowed due to the assertion script:

assertionScript := fmt.Sprintf("expected: %s\n#Actual: %s\nunified: #Actual & expected\n", expectedVal, actualBytes)

In this, #Actual is a definition and is closed by default. This is a recursive property, so openness will only apply where one of the nested structs opens via ..., which does not apply recursively -- which seems like a fair solution?.

There is a test in this PR that validates that extra fields within a resource results in an error (and the same facility also catches any higher-leveled keys that are not in the actual response as well.

@gotwarlost
Copy link
Collaborator

gotwarlost commented May 22, 2025

Cool. I missed that nuance.

Since every "diff" mode test is also a valid "unification" mode test, we can support a different annotation like, say, @complete which enables diff mode in addition to unification mode and prints the diffs. This would be backwards-incompatible in a minor way (we can bump the minor version for the new release) but with a clear migration path where people can just add the @complete annotation to their tests which will preserve existing behavior.

I think being able to specify partial responses should be the default mode of operation going forward so that unit tests are truly like, er, unit tests :)

@gotwarlost
Copy link
Collaborator

In fact there is a third mode of operation where a test writer would be able to add a @complete annotation to a specific resource in the response and the tester would activate a diff test just for that resource.

Not suggesting we implement this in this PR but it allows for that possibility.

foo: "bar"
}

#mode: "unification"
Copy link
Author

Choose a reason for hiding this comment

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

This was left in as an omission, based on thinking around whether it would be nicer to represent it as a definition instead of an attribute. An attribute leads itself nicer to partial unification as mentioned in another top-level PR comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants