Detect errors

[IlanCosman]

Sometimes a test may pass but errors are printed to the terminal. This would be a "failure" from a human perspective. Thus, it would be nice if Fishtape could detect if errors are printed, and fail accordingly.

[jorgebucaran]

The best we can do is an exit as soon as we encounter the first error (not failure). Did you have something else in mind?

[IlanCosman]

That sounds perfect! I don't see any reason that authors would intend for errors to appear in their tests without redirecting them to stdout so that they can be tested by fisher. 👍

[jorgebucaran]

We could lint the test file first and exit if that fails (if there are syntax errors for example), but we can't do much beyond that.

[IlanCosman]

I meant something like this:

@test "echo hello if bar isn't baz" (test $bar = "baz" || echo hello) = "hello"

This would error if $bar isn't defined, but would still echo hello and thus Fishtape would still mark it as ok. I'm thinking we could redirect stderr to something (a file?) and then fail if it we detect anything.

[jorgebucaran]

I would consider this a form of magic. While arguably convenient, the true behavior of the command substitution is not immediately apparent since Fishtape can change how Fish is supposed to work. The answer is to just " " wrap $bar.

Wouldn't it be great if fish --no-execute displayed a warning when a variable used in a test expression is not enclosed in double-quotes? Fish already goes out of its way in the test man page to mention this:

When using a variable as an argument for a test operator you should almost always enclose it in double-quotes. [...]

I don't think we can completely solve this without creating a trade-off elsewhere.

[IlanCosman]

While arguably convenient, the true behavior of the command substitution is not immediately apparent since Fishtape can change how Fish is supposed to work.

Could you explain this sentence further? I don't quite understand what you're saying.

I am in fact suggesting some magic, but that's not a bad thing. Fishtape is a test harness, and some magic is to be expected when it helps the user catch bugs. Fishtape is lacking in magic, @test doesn't really offer much more than test.

I think this would be a valuable addition, and I'd be happy to implement it if you're interested. If not that's okay too 😄

[jorgebucaran]

I think this would be a valuable addition, and I'd be happy to implement it if you're interested.

I'm interested! Maybe it's not what I'd call magic at all. You are welcome to send me a PR.

... @test doesn't really offer much more than test.

That's fine! I'd much rather avoid lock-in. I don't want users to "learn" Fishtape or write Fishtape tests. Instead, I want users to write Fish tests. Fishtape should only facilitate this.

Earlier versions of Fishtape were replete with magic. I'd often find people using it in strange ways, sometimes to work around apparent bugs I'd never heard of. Simple makes your tests easier to debug because it's usually you, not Fishtape that erred.

Could you explain this sentence further? I don't quite understand what you're saying.

I want to fairly guess how my test will do just by looking at the code. Magic makes this more difficult. That's all. 😁

[IlanCosman]

Here's a more real-world example: https://github.com/IlanCosman/tide/runs/1764410359?check_suite_focus=true

The pwd item produces errors on macOS that it doesn't on Linux. I'm thinking it's a seq difference perhaps, but I haven't been able to work on it yet.

EDIT: Yup, it was seq. seq 0 on Linux prints nothing. seq 0 on macOS prints 1 and 0.

[edouard-lopez]

I have a case in pure CI, where I run a PR's tests against Fish 3.3.1 and have errors due to the use of $(cmd) syntax (only supported since 3.4.0 yet the job is in success cause Fishtape didn't detect the problem.

Fish error

~/.config/fish/pure/tests/../functions/_pure_prompt_k8s.fish (line 5): $(...) is not supported. In fish, please use '(_pure_k8s_conte…)'.
        and test -n "$(_pure_k8s_context)"

I was expecting Fishtape to exit with a non-zero status code. Is there a strategy I could adopt to bubble this error to the job?

[edouard-lopez]

I noticed the total number of tests is off when there is a shell error vs. when there is none.

Not sure we can use the occurrence count of @test as sometimes they are conditioned (e.g. https://github.com/pure-fish/pure/blob/924c1c6a9c88125d8d58f00ffe5816fed57f4d99/tests/_pure_prompt_vimode.test.fish#L40-L49)

[jorgebucaran]

Hmm, it seems that the tests weren't counted because they didn't run.

[edouard-lopez]

Got another false positive example. Looking at the github check, it seems ok

But when opening the results, there is in fact failure (the variable was not set for the test)

ok 149 _pure_prompt_current_folder: fails if no argument given
test: Missing argument at index 3
-ge 1
      ^
~/.config/fish/pure/tests/../functions/_pure_parse_directory.fish (line 14): 
    if test $pure_truncate_prompt_current_directory_keeps -ge 1
       ^
in function '_pure_parse_directory' with arguments '9'
	called on line 1 of file ~/.config/fish/pure/tests/../functions/_pure_prompt_current_folder.fish
in command substitution
	called on line 7 of file ~/.config/fish/pure/tests/../functions/_pure_prompt_current_folder.fish
in function '_pure_prompt_current_folder' with arguments '10'
	called on line 1 of file ~/.config/fish/pure/tests/_pure_prompt_current_folder.test.fish
in command substitution
	called on line 8 of file ~/.config/fish/pure/tests/_pure_prompt_current_folder.test.fish
in command substitution
	called on line 23 of file ~/.config/fish/pure/tests/_pure_prompt_current_folder.test.fish

I agree to fix is simple, but spotting the issue requires manual checks on the test output

[jorgebucaran]

Did you fix the test? Please share one example of a failed test and how you fixed it.

[edouard-lopez]

@jorgebucaran I declared the missing variable and added quoted on the if test …. It was something similar to was @IlanCosman explained in #60 (comment)