Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: increase _fishtape_test_failed when file is invalid #66

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

edouard-lopez
Copy link
Contributor

@edouard-lopez edouard-lopez commented Jan 24, 2024

related: #60

Here, I reproduce a case of silent error I got on pure due to an invalid file. The change was made through Github UI and suggestion feature so no linting was done.

Test

source ./functions/fishtape.fish; fishtape ./tests/invalid.fish

CI

Not sure how to run this on CI

@edouard-lopez edouard-lopez force-pushed the feat/detect-malformed-files branch from 5930860 to 7fca022 Compare January 24, 2024 15:29
@jorgebucaran
Copy link
Owner

-z expects a string. Do we want your test to fail when no string is provided to -z?

@edouard-lopez edouard-lopez force-pushed the feat/detect-malformed-files branch from 7fca022 to f7cbca0 Compare January 24, 2024 16:27
source ./functions/fishtape.fish; fishtape ./tests/invalid.fish

see jorgebucaran#60
@edouard-lopez
Copy link
Contributor Author

I updated the test with a tautology. The invalid file should be considered a failed test, so the pipeline doesn't succeed.

Below, I highlighted the output changes

❯ source ./functions/fishtape.fish; fishtape tests/invalid.fish                                                                                                                                  ✖️
TAP version 13
~/projects/contributions/fishtape/tests/invalid.fish (line 4): 'end' outside of a block
end # we want to trigger a parsing error
^~^
warning: Error while reading file /home/edouard/projects/contributions/fishtape/tests/invalid.fish


1..0
# pass 0
-# ok
+# fail 1

@jorgebucaran
Copy link
Owner

Isn't that the same situation with -z? = expects two strings.

@edouard-lopez
Copy link
Contributor Author

The @test is not important, but an invalid file should trigger a failed exit code so pipeline jobs fails too and are visible as such.

@jorgebucaran
Copy link
Owner

Are we not currently failing tests when the user misuses -z or =? Does this mean some tests are passing even when a flag is used incorrectly?

@edouard-lopez
Copy link
Contributor Author

You can check out the result of fishtape tests/invalid.fish with the current Fishtape version, it's not failing.

@jorgebucaran
Copy link
Owner

I thought the expectation was for incorrect flag usage to result in a failure.

I can see invalid.fish ends with an end that's not valid in Fishtape, correct?

@edouard-lopez
Copy link
Contributor Author

Yep, it's on purpose, so the file is invalid in Fish and it triggers a parsing error.

Previously, Fishtape didn't increment the _fishtape_test_failed variable in such case and thus resulted in a success (despite the existence of an error).

The behaviour I introduce aligns the exit code of Fishtape with the reality of Fish parsing

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.

2 participants