test: test fail-argument

[sebastiaanspeck]

No description provided.

[sebastiaanspeck]

This PR demonstrates that setting fail to false does not work ATM.

[YDX-2147483647]

The cause of this issue is trivial: $INPUT_FAIL gets changed to true because of failIfEmpty: true, which is the default. The line INPUT_FAIL=true was introduced in #86.

lychee-action/entrypoint.sh

Lines 34 to 39 in ae46991

	# Overwrite the exit code in case no links were found
	# and `failIfEmpty` is set to `true` (and it is by default)
	if [ "${INPUT_FAILIFEMPTY}" = "true" ]; then
	# Explicitly set INPUT_FAIL to true to ensure the script fails
	# if no links are found
	INPUT_FAIL=true

lychee-action/action.yml

Lines 17 to 19 in ae46991

	failIfEmpty:
	description: "Fail entire pipeline if no links were found"
	default: true

To verify it, I set failIfEmpty: false to the step in test.yml, and now it does pass. Directly adding echo "$INPUT_FAIL" also confirms it.

  - name: test disable fail - it's okay if we fail
    uses: ./
    with:
      args: --no-progress foo.bar
      fail: false
+     failIfEmpty: false

Thanks https://github.com/nektos/act for making it a lot easier to debug.

Possible fix

Document this behaviour, or edit entrypoint.sh:

	# Overwrite the exit code in case no links were found
	# and `failIfEmpty` is set to `true` (and it is by default)
	if [ "${INPUT_FAILIFEMPTY}" = "true" ]; then
-	    # Explicitly set INPUT_FAIL to true to ensure the script fails
-	    # if no links are found
-	    INPUT_FAIL=true
	    # This is a somewhat crude way to check the Markdown output of lychee
	    if grep -E 'Total\s+\|\s+0' "${LYCHEE_TMP}"; then
	        echo "No links were found. This usually indicates a configuration error." >> "${LYCHEE_TMP}"
	        echo "If this was expected, set 'failIfEmpty: false' in the args." >> "${LYCHEE_TMP}"
	        exit_code=1
+	        # Explicitly set INPUT_FAIL to true to ensure the script fails
+	        # if no links are found
+	        INPUT_FAIL=true
	    fi
	fi

I can make a PR if wanted.

[mre]

Thanks for the investigation. I'm all for changing the code here. The way I see it,

• If fail: false and failIfEmpty is not set, don't fail if the status code is not 0 but fail if links were found.
• If fail is not set, and failIfEmpty: false, fail if the status code is not 0 but don't fail if there are no links.
• If none is set, use the defaults: fail if the status code is not 0 or no links were found.

Please correct me if I'm wrong.

I'd appreciate a pull request.

[YDX-2147483647]

Maybe this PR can be merged first?
If #262 needs more tests, but this PR is not merged, then there will be a merge conflict later.

[mre]

Maybe this PR can be merged first?

Do you mean this PR? (251)

Because it currently fails.
I checked out your PR (#262) and that passes and seems to fix the issue, no? I think we can close this one (251) and merge yours. Or am I missing something?

[mre]

Ah, I think I understand now. Can you merge the changes of this PR into your branch? Then we have all changes (including a passing test) in one pull request. I prefer that, because it's a single atomic change and we can make sure it works. I'd rather not merge a change if the pipeline is red. 😉

[mre]

@sebastiaanspeck, this PR got merged as part of #262 by @YDX-2147483647.
Good work by both of you. 🎉