nextest-rs / nextest

A next-generation test runner for Rust.
https://nexte.st
Apache License 2.0
2.17k stars 96 forks source link

Bug: filterset binary operators are parsed as function applications #1816

Open thoughtpolice opened 3 days ago

thoughtpolice commented 3 days ago

Description of the issue

Description: When running the Jujutsu test suite, I'm seeing a flaky test I would like to skip. I tried using the following command after skimming the filterset documentation:

austin@GANON:~/src/jj$ cargo nextest run --workspace -j 6 -E 'not(test(test_shallow_commits_lack_parents))'

However, this fails with a rather confusing error, because the not operator is (I am assuming completely based on no reading of the code) parsed as a function not() which doesn't exist.

Simply adding a space not (...) in the predicate does what I expect and succeeds.

Expected outcome

I expect the test suite to run, and skip the singular test named test_shallow_commits_lack_parents

Actual result

  error: expected expression
   ╭────
 1 │ not(test(test_shallow_commits_lack_parents))
   · ──────────────────────┬─────────────────────
   ·                       ╰── missing expression
   ╰────

  error: expected end of expression
   ╭────
 1 │ not(test(test_shallow_commits_lack_parents))
   · ──────────────────────┬─────────────────────
   ·                       ╰── unparsed input
   ╰────

error: failed to parse filterset

Nextest version

austin@GANON:~/src/jj$ cargo nextest --version
cargo-nextest 0.9.79
release: 0.9.79
host: x86_64-unknown-linux-gnu

Additional context

I assume this is some kind of grammar nit, though I'm no expert in parsing, but ideally there should be:

  1. Some fix to the grammar to allow this (i.e. parsing binary operators as a separate term). Maybe you could hack it by also defining every binary operator as a 1-arity function :)
  2. At least some documentation on this, if nothing else.
  3. A better error message? I puzzled on this for a solid 5 minutes. Not sure what can be done here.

Also, I note that nextest 0.9.80 added (back) support for --skip, presumably in terms of a filterset-under-the-hood. That would have let me work around this, but this still remains.

sunshowers commented 3 days ago

Ahh that's definitely confusing. I'm definitely tempted to tokenize something like not separately, but I'm worried people are going to be led down an incorrect path, and start thinking of all the boolean operators as prefix operators. A better error message is definitely valuable here.

And yes, --skip now works :)