Skip to content

Conversation

@hudlow
Copy link

@hudlow hudlow commented Dec 5, 2025

This reworks how we handle tests a little, adding inferred types that we can use to test the upcoming type-checker implementation.

packages/cel-spec/README.md explains the philosophy thusly:

The tests aggregated by this package are useful for incremental testing of a CEL implementation. That is, they provide input expressions along with outputs at multiple points in the lifecycle of a CEL program, not only its final output.

Chiefly, tests provide some or all of the following outputs:

  • The parsed abstract syntax tree (AST) for an input expression
  • The output type that should be statically inferred by a type checker for an expression
  • The final result of compiling and running the expression

It should be noted that only the final result is actually provided by the conformance tests sourced from the CEL specification repo. The incremental outputs are derived from the cel-go implementation, considering it to be positioned as a reference implementation.

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

One comment from me, see below. I also opened a small clean-up PR based on this. This looks good to me.

Comment on lines 44 to 50
export interface IncrementalTest {
name: string;
original: SimpleTest;
ast?: string;
type?: string;
error?: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

The upstream SimpleTest definition already provides means to expect an error, and to expect a type. See https://github.com/google/cel-spec/blob/v0.25.1/proto/cel/expr/conformance/test/simple.proto#L109 and https://github.com/google/cel-spec/blob/v0.25.1/proto/cel/expr/conformance/test/simple.proto#L132.

They are used in the CEL conformance test data, see https://github.com/google/cel-spec/blob/v0.25.1/tests/simple/testdata/lists.textproto#L72 and https://github.com/google/cel-spec/blob/v0.25.1/tests/simple/testdata/type_deduction.textproto#L17.

So our generated test data ends up with redundant information, for example the expectation that the expression "true" is type bool:

{
  original: {
    name: "bool",
    expr: "true",
    typedResult: {
      result: { boolValue: true },
      deducedType: { primitive: "BOOL" },
    },
  },
  ast: "true^#*expr.Constant_BoolValue#",
  type: "bool",
}

I wish this could be consolidated. If we can't right now, the least we should do is to add documentation to the properties of the IncrementalTest type.

Copy link
Author

Choose a reason for hiding this comment

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

@timostamm I went ahead and added documentation.

Comment on lines 44 to 47
// An `IncrementalTest` supplements a test extracted from the conformance suite
// or from the `cel-go` source code with additional information derived from the
// `cel-go` implementation.
export interface IncrementalTest {
Copy link
Member

Choose a reason for hiding this comment

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

That's great info, thanks!

Can we use JSDoc blocks? E.g. https://github.com/bufbuild/cel-es/blob/v0.3.0/packages/cel/src/timestamp.ts#L18-L21

I realize now that the doc style isn't consistent in this repository 🙈 JSDoc blocks are for user-facing docs (but we don't use JSDoc tags). Line comments are for internal implementation notes.

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.

3 participants