cmd/cue: vet's -c flag may not be working as documented

[mvdan]

What version of CUE are you using (cue version)?

$ cue version
cue version v0.5.0-beta.1.0.20221119112418-32bfffd0aa53

Does this issue reproduce with the latest release?

Yes; v0.4.3 has the same output.

What did you do?

exec cue vet schema.cue data.json
! exec cue vet -c schema.cue data.json

exec cue vet schema.cue data.cue
! exec cue vet -c schema.cue data.cue
-- schema.cue --
foo: string
bar: string
-- data.cue --
foo: "x"
-- data.json --
{
    "foo": "x"
}

What did you expect to see?

From cue vet -h, I see:

vet validates CUE and other data files

By default it will only validate if there are no errors.
The -c validates that all regular fields are concrete.
[...]

So I would assume that cue vet would only check errors, and cue vet -c would check for errors and require the resulting values to be concrete.

So, cue vet schema.cue data.json should succeed, but cue vet -c schema.cue data.json should fail as the value is not concrete.

I would expect the same if the data is in CUE format rather than JSON. Any arguments being non-CUE files does make vet behave in a different mode, but as far as I can tell, that shouldn't affect whether we require concreteness (or completeness?).

What did you see instead?

All four commands fail:

$ testscript -v repro-cmd.txtar 
[...]
> exec cue vet schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
> ! exec cue vet -c schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
> exec cue vet schema.cue data.cue
[stderr]
some instances are incomplete; use the -c flag to show errors or suppress this message
[exit status 1]
> ! exec cue vet -c schema.cue data.cue
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]

cc @rogpeppe

[myitcv]

So, cue vet schema.cue data.json should succeed

I've updated the repro to reflect this expectation, and for the data.cue case too.

Indeed, just to confirm my understanding, does this reflect the expectation?

What did you do?

# With -c flag
! exec cue vet -c schema.cue data.json
cmp stderr stderr.golden
! exec cue vet -c schema.cue data.cue
cmp stderr stderr.golden

# No -c flag
exec cue vet schema.cue data.json
! stdout .+
exec cue vet schema.cue data.cue
! stdout .+

-- schema.cue --
foo: string
bar: string
-- data.cue --
foo: "x"
-- data.json --
{
    "foo": "x"
}
-- stderr.golden --
bar: incomplete value string:
    ./schema.cue:2:6

What did you expect to see?

A passing test.

What did you see instead?

$ ts repro.txtar
# With -c flag (0.027s)
# No -c flag (0.015s)
> exec cue vet schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
FAIL: /tmp/testscript2980766130/repro.txtar/script.txtar:8: unexpected command failure
> exec cue vet schema.cue data.cue
[stderr]
some instances are incomplete; use the -c flag to show errors or suppress this message
[exit status 1]
FAIL: /tmp/testscript4206477420/repro.txtar/script.txtar:10: unexpected command failure

(notice I have hand-crafted the output, because testscript ordinarily fails on the first failure)

i.e. the bug here is not with -c: that appears to be working as expected.

Rather that without -c, we still get errors when we should not, correct?

[mvdan]

Rather that without -c, we still get errors when we should not, correct?

Correct.

(notice I have hand-crafted the output, because testscript ordinarily fails on the first failure)

I can see how it is better from a clarity perspective to represent the desired behavior in the testscript, not the current behavior. It's just hard to do that currently, as testscript stops at the first unexpected command result. That's why I tend to write a testscript which passes, even if that reflects the current bad behavior.

Perhaps it would be enough for testscript to support ? - then our reproducer could use that in the last two commands, allowing them to show either success or failure, but never stop the execution.

Either way, it would be nice to clarify this in https://github.com/cue-lang/cue/wiki/Creating-test-or-performance-reproducers. That is, when filing a reproducer, should the testscript represent the desired or the current behavior?

[mvdan]

It's perhaps also worth noting that the thread is a bit confusing now that we have two slightly different test scripts and manually crafted outputs :) It's fine by me for you to continue editing my original post - then there's no need to have a sibling in a comment.

[myitcv]

It's just hard to do that currently, as testscript stops at the first unexpected command result.

Which is why I was pondering a mode for cmd/testscript that allows execution to continue in the presence of errors, just logging the result. But that's very much unrelated to this issue.

That's why I tend to write a testscript which passes, even if that reflects the current bad behavior.

I find this counterintuitive given that the heading is "What did you do?", not "What is the current behaviour?". As a reader, my expectation is therefore that I will be looking at a reproducer that shows me what the user tried to do. The heading that follows, "What did you expect to see?" then reflects their expectation given the reproducer. "What did you see instead?" represents the reality.

Perhaps it would be enough for testscript to support ? - then our reproducer could use that in the last two commands, allowing them to show either success or failure, but never stop the execution.

I'm not clear that this helps, because ? doesn't say anything about the expectation of whether the command should pass or not. And that's the whole point in this case (along with the output to stderr). It might help for the construction of the repro, but I think it confuses the actual bug report.

That is, when filing a reproducer, should the testscript represent the desired or the current behavior?

Is that not clear from the questions in the bug report, per my comments above? I don't think it's specific to the wiki, rather the bug template itself. So if that can be improved we should do it there.

[mvdan]

allows execution to continue in the presence of errors

I guess its usefulness would be rather limited, because some errors really are fatal and will break the following commands in basic ways. That's why I was thinking of ? - it doesn't set an expectation either way, but it does signal "this result is not what I would expect".

It might help for the construction of the repro, but I think it confuses the actual bug report.

Sure, it's a bit of a tradeoff and I agree that clarity probably wins here. Though another aspect of manually constructing the testscript is that I can't run the whole thing to see what it does, as it just stops :) but perhaps that's solved by the same change we're talking about above.

Is that not clear from the questions in the bug report, per my comments above?

Well, "what did you do" says nothing about whether the reproducer testscript should "PASS" or "FAIL". I default to "PASS to reproduce the bug", and you default to "PASS once the bug is fixed per my expectation". I don't think the template nor the wiki page point towards one method or the other. I did follow "what did you expect to see" (I expected to see "FAIL") and "what did you see instead" (I saw a "PASS", with the provided output).

I don't have a particularly good idea about what the template could look like, yet. But where we explain how to write testscripts is the wiki page, so my intuition is that the expected result of running the testscript should be defined there too.

[myitcv]

FYI for anyone following along. Much of the exchange above will be dramatically improved by https://github.com/rogpeppe/go-internal/pull/189.

[myitcv]

https://github.com/rogpeppe/go-internal/pull/189 has landed, which makes it easier to express what's going on here in the form of test that is expected to pass.

What did you do?

Ran:

testscript -continue <<EOD
# With -c flag
! exec cue vet -c schema.cue data.json
! exec cue vet -c schema.cue data.cue

# No -c flag
exec cue vet schema.cue data.json
exec cue vet schema.cue data.cue

-- schema.cue --
foo: string
bar: string
-- data.cue --
foo: "x"
-- data.json --
{
    "foo": "x"
}
EOD

(notice use of the new -continue flag)

What did you expect to see?

Passing test.

What did you see instead?

# With -c flag (0.027s)
# No -c flag (0.025s)
> exec cue vet schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
FAIL: /tmp/testscript1217859574/-/script.txtar:6: unexpected command failure
> exec cue vet schema.cue data.cue
[stderr]
some instances are incomplete; use the -c flag to show errors or suppress this message
[exit status 1]
FAIL: /tmp/testscript1217859574/-/script.txtar:7: unexpected command failure
error running <stdin> in /tmp/testscript1217859574/-

[jpluscplusm]

Rather that without -c, we still get errors when we should not, correct?

I've just been bitten by exactly this.

I've tended to use cue vet as a signal for "have I done anything, thus far, that CUE can tell me is /definitively/ wrong?", often as a mid-implementation-journey lightweight check. I use cue vet -c in a distinctly different scenario, to ask "have I done /enough/ that I'll be able to emit structures into non-CUE-aware tooling?". I've also used cue vet as a check for unit-testing-CUE-"functions" ("Functional CUE", not actual functions) - a setup which doesn't work so long as cue vet, without -c, has exit-code-affecting opinions about concreteness.

The stderr warning given for non-concrete values when -c isn't used is fine; but the resulting error code being non-zero solely because of the non-concreteness -- when I feel I didn't ask for that check to be performed -- is quite problematic. It effectively stops cue vet being used in various testing scenarios.

The discussion, above, of using a new testscript feature is (IMHO!) onerous. Requiring the consumer to have testscript also installed to work round the problem isn't a great fix; especially when these kinds of tests are being run in Enterprise-level CI/CD pipelines, where the process of on-boarding new tooling is often /highly/ non-trivial! I /may/ have misread the testscript feature's discussion (perhaps it's a meta discussion, not affecting the cue-vet-c problem?), but I thought it worth explicitly making the point that the problem should -- IMHO! -- most properly be addressed in the cue CLI :-)

[mvdan]

The discussion about testscript was purely about providing a useful and self-describing reproduction script :) it has nothing to do with the fix itself.

[mvdan]

@rogpeppe pointed out that the flag currently has three states:

https://github.com/cue-lang/cue/blob/48207fb5fbfd85784c0560ea32c559183b49f475/cmd/cue/cmd/vet.go#L113-L118

That is, the presence of the flag changes behavior, it's not just its true/false value.

So we then have three modes currently:

• Without any -c flag, we get the default behavior, currently documented as "only validate if there are no errors". This mode can trigger some instances are incomplete; use the -c flag to show errors or suppress this message.
• With -c or -c=true, we require the evaluation to be concrete, showing all the non-concrete errors.
• With -c=false, we don't require the evaluation to be concrete at all.

I wasn't aware of the default mode being neither true or false when I raised this issue; I naively understood that the boolean flag would work like most others, where there are just two modes and the default is false.

I would still leave this issue open. If the current behavior is desirable, it should be better documented, because I completely misunderstood it until Roger pointed me at the code. I would argue that the default behavior should be tweaked as well, though, per the discussion above; cue vet shouldn't fail on incomplete errors at all.

[mvdan]

My last comment here is correct - I still think the three modes are slightly confusing and could be better documented - but we should focus on the bug at hand with cue vet (without a flag) first. That is what Paul showed in https://github.com/cue-lang/cue/issues/2120#issuecomment-1325420796: cue vet appears to show either errors or warnings relating to incomplete values, and return an exit code of 1, even though we didn't specify the -c flag.