add tests, deal with pipes, newlines, tabs for to nuon

[merelymyself]

Description

Related: #6379

Tests

Make sure you've done the following:

• Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
• Try to think about corner cases and various ways how your changes could break. Cover them with tests.
• If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

• cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• cargo test --workspace --features=extra to check that all the tests pass

[sholderbach]

Do we have code already to escape them to \t and \n respectively?

[kubouch]

I agree, the allowed / disallowed characters are already handled by the parser. If we want to disallow some characters in column names, let's do it in the parser. Here, we should just check for characters that break the to nuon | from nuon roundtrip and wrap them in quotes.

[kubouch]

I'd propose the following methodology:

1. Disable quoting entirely (for now)
2. Loop through all ASCII characters and check for
   a) [ [ $'foo($char)bar' ]; [ 'test' ] ] | to nuon | from nuon
   b) { $'foo($char)bar': 'test' } | to nuon | from nuon
3. Collect the characters that cause 2a) or 2b) to fail
4. Enable quoting for the collected characters

The step 2. can be put into the test suite (excluding obvious working characters like 0-9, a-z or A-Z).

[merelymyself]

I used proptest, as suggested in the other PR. It's a dev-dependency, so that should be fine, right?

[sholderbach]

I used proptest, as suggested in the other PR. It's a dev-dependency, so that should be fine, right?

Does it execute growing and shrinking in the CI or do you trigger the expensive operation manually?
(It has been a while that I used it)

[sholderbach]

2. Loop through all ASCII characters and check for
   a) `[ [ $'foo($char)bar' ]; [ 'test' ] ] | to nuon | from nuon`
   b) `{ $'foo($char)bar': 'test' } | to nuon | from nuon`

I would add
a) [ [ $'($char)' ]; [ 'test' ] ] | to nuon | from nuon
b) { $'($char)': 'test' } | to nuon | from nuon

for the particularly pathological cases

Also with proptest doing strings should be fine and that will discover some other pathologies like

{"(1)": one} that will round trip to {1: one} if there are no quotes

[kubouch]

Yeah, we can also add the single-char cases @sholderbach suggested. I've never used proptest -- does it test all the characters? I don't see the logic from this PR's commits.

[merelymyself]

@kubouch - I don't think proptest tests every character, but it does cast a rather wide net over often-problematic characters and strings.

@sholderbach - I've added the cases, thanks for the suggestion! {"(1)": one} doesn't roundtrip because anything with brackets is quoted automatically. As for your first question, sorry, I don't quite understand what you mean.

[merelymyself]

I also found an unrelated parser issue, #6401

[sholderbach]

Looking good to me. Thanks @merelymyself

I was wondering if it will sample new tests during each call to CI or if it will only check the previous failures. I didn't notice a big impact on the CI times so that should probably not be an issue (as long as we don't discover random failuers over time in unrelated PR's)
Do we have the machinery to escape/quote the caught cases already or do we error out in those case for now?

Other than that would be happy to land that.

[merelymyself]

I believe it will sample new tests, but I've ran it as much as I could, so I can only hope that some new issue won't be suddenly discovered.

The caught cases are already quoted - for weird cases where " or the null byte is chosen as a string, it errors out, and the test has built-in catches for that.

[sholderbach]

OK yeah let's land it. We can probably deal with a bit of fuzzing happening in the background ^^
I would consider the null byte outside of the realm of things we should handle gracefully when most programs will terminate strings on it...
Escaping the double quotation mark should probably be on the list of things we deal with properly but if we for now fail safe with to nuon I wouldn't consider that a stopper for this PR

[rgwood]

Proptest is very cool but I have noticed that these tests are much slower than others.

cargo nextest run --all runs about 2000 individual tests, and on a fast desktop it takes ~12s with the 2 proptests and ~6s without. 2 out of 2000 tests are responsible for consuming 50% of test time!

I imagine the slowdown is worse on slower machines like the test runners. Not sure what the best way forward is (but also it's not especially urgent).

[sholderbach]

Yeah, running the extra iterations on proptest or quickcheck in CI doesn't seem to catch anything new. Either we restrict the iterations (at least with proptest that's configurable), ignore the tests and move the reproducibles to regular tests, or do feature crimes... (with the second approach you would have to name the test to run sampling)
I had a few other things I would like to stress test. But yeah that shouldn't run on every PR.
We could think about a cron workflow with just the fuzzing and some longer iteration time.

(what is the state on switching nextest back on?)

[rgwood]

(what is the state on switching nextest back on?)

Not sure. IIRC there were 2 main issues with nextest in CI:

1. Didn't notice a huge speedup in CI with nextest (maybe because measuring CI is hard, maybe because our CI runners only have 2 cores?)
2. nextest doesn't support doctests so we still needed to run cargo test for a few tests

We could probably turn it on again, but the cost-benefit analysis is kinda ¯\_(ツ)_/¯