Skip to content

Add regression test for boolean input with a default (#779)#2288

Draft
arimu1 wants to merge 1 commit into
common-workflow-language:mainfrom
arimu1:fix/779-boolean-false-default-not-required
Draft

Add regression test for boolean input with a default (#779)#2288
arimu1 wants to merge 1 commit into
common-workflow-language:mainfrom
arimu1:fix/779-boolean-false-default-not-required

Conversation

@arimu1

@arimu1 arimu1 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses #779.

Issue #779 reported that a boolean input with default: false was incorrectly treated as a required CLI argument:

$ cwltool t.cwl
t.cwl: error: the following arguments are required: --a_false

…while default: true worked fine — an argparse-generation asymmetry.

Status: already fixed, never locked down

This was fixed in 48e8afd (Aug 2019), when the required computation in add_argument became:

required = default is None and input_required

Since a boolean with default: false has default=False (not None), it is correctly optional today. I confirmed current main runs the issue's repro without error. However, the behavior was never covered by a test and the issue is still open.

Change

Adds a parametrized regression test in tests/test_toolargparse.py (plus two small fixtures under tests/wf/) covering both default: false and default: true. It builds the parser via generate_parser(..., input_required=True) — mirroring the default CLI behavior — and asserts that parsing with no arguments succeeds and falls back to the declared default, rather than erroring as required.

I verified the test genuinely guards the bug: against the pre-2019 logic it fails on the default: false case with the exact error from the issue, while the default: true case still passes — matching the original report.

$ python -m pytest tests/test_toolargparse.py -q
19 passed

black, flake8, and isort are clean on the changed files. Once this lands, #779 can be closed.


This change was produced with the assistance of Claude Code (model: Claude Opus). The analysis and test were reviewed by a human before submission.

@mr-c mr-c marked this pull request as draft June 25, 2026 13:35
@arimu1 arimu1 force-pushed the fix/779-boolean-false-default-not-required branch from 69d5858 to c4aec7d Compare June 28, 2026 06:38
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.18%. Comparing base (5634342) to head (14cac33).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2288   +/-   ##
=======================================
  Coverage   85.18%   85.18%           
=======================================
  Files          46       46           
  Lines        8593     8593           
  Branches     2011     2011           
=======================================
  Hits         7320     7320           
  Misses        807      807           
  Partials      466      466           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arimu1 arimu1 force-pushed the fix/779-boolean-false-default-not-required branch 2 times, most recently from fe6354f to 86844c3 Compare June 28, 2026 10:36
…-language#779)

Issue common-workflow-language#779 reported that a boolean input with `default: false` was
treated as a required CLI argument (`error: the following arguments
are required: --a_false`), while `default: true` worked fine.

This was fixed in 48e8afd (2019) when `required` became
`default is None and input_required`, but the behavior was never
locked down by a test and the issue stayed open.

Add a parametrized regression test in test_toolargparse.py covering
both `default: false` and `default: true`, asserting the generated
parser treats the input as optional and falls back to the declared
default. Verified the test fails against the pre-fix logic with the
exact error from the issue.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@arimu1 arimu1 force-pushed the fix/779-boolean-false-default-not-required branch from 86844c3 to 14cac33 Compare July 11, 2026 03:17
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.

1 participant