Skip to content

Add basic conformance tests for the sets extension - #533

Merged
jnthntatum merged 2 commits into
cel-expr:masterfrom
jnthntatum:feat/sets_conformance
Oct 21, 2026
Merged

jnthntatum merged 2 commits into
cel-expr:masterfrom
jnthntatum:feat/sets_conformance

Conversation

@jnthntatum

Copy link
Copy Markdown
Collaborator

No description provided.

@jnthntatum
jnthntatum requested a review from l46kok October 21, 2026 19:06

@l46kok l46kok left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, just some minor suggestions for more coverage

expr: "sets.contains([{'a': 'b'}, {'c': 'd'}], [{'a': 'b'}])"
}
test: {
name: "maps_heterogeneous_numeric"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we also add a case for heterogeneous map keys?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

added a basic one. there are some behavior inconsistencies when there's an equivalent uint and int so leaving that out for now.

# Returns whether the first list argument (arg0) contains all elements in the
# second list argument (arg1), i.e., whether arg0 is a superset of arg1.
section: {
name: "contains"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Test cases that exercises the numeric boundaries would be nice (NaN, max int, max uint etc)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

test: {
name: "proto_messages"
container: "cel.expr.conformance.proto3"
expr: "sets.equivalent([TestAllTypes{single_int64: 1, single_uint64: 2u}, TestAllTypes{single_int64: 1, single_uint64: 2u}], [TestAllTypes{single_int64: 1, single_uint64: 2u}])"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thoughts on adding wrappers? Just to make sure the WKT handling works as intended

sets.equivalent(
  [TestAllTypes{single_int64_wrapper: 1}.single_int64_wrapper, TestAllTypes{}.single_int64_wrapper],
  [null, 1u]
)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

@jnthntatum
jnthntatum requested a review from l46kok October 21, 2026 21:02
@jnthntatum
jnthntatum merged commit e344515 into cel-expr:master Oct 21, 2026
8 checks passed
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