Assert error message content in tests #37

h4l · 2022-08-28T09:19:47Z

I'm going to have a crack at making the error messages a little more user-friendly, to resolve #5. Especially the Warning: found unmatched (duplicate?) arguments [Argument(None, '.\\10. PowerShell.ps1')] that occurs in response to an end user passing incorrect or too many arguments when running a program using docopt-ng. I need that to be resolved before I can use docopt-ng in place of docopt really.

Before making changes there, I'd like to ensure the current messages are tested, so I can be sure I don't change messages unexpectedly.

This PR mostly adds exception message assertions to all the places where tests trigger an exception or SystemExit with usage.

The first 3 commits are unrelated to these messages assertions — small improvements to the dev experience using this repo. They could go into a separate PR if you prefer, but I've already opened a few, so I didn't want to swamp with too many!

h4l · 2022-08-28T09:26:42Z

h4l force-pushed the test-error-messages branch from 1f166be to 93ea3f0 35 seconds ago

Removed import re from one of the commits.

h4l · 2022-08-28T14:45:12Z

Also, just to flag that one of the commits (ecc12c2) makes a change to the GitHub Actions config.

h4l · 2022-09-01T18:08:17Z

h4l force-pushed the test-error-messages branch from 93ea3f0 to 9c0f189 1 minute ago

Fixed tests failing on Python 3.7 due to annotation with dict[...]. Also changed "use a stable sys.argv" to use list instead of a tuple for the patched argv — docopt expects it to be a list.

And also just added:

fix: detect multi-word options sections in usage
fix: handle 3+ word Options: section names

We'd lost a line of coverage after "test: assert the messages of raised errors" as no test was triggering the "options (case-insensitive) was found in usage" error. I added a test to trigger it, and in doing so realised the code checking for the error condition was not handling options sections with extra qualifying words, like "Advanced Options:". And further to that, a separate error occurred with 3+ word Options sections.

tests/test_docopt.py

NickCrews · 2022-09-07T23:08:26Z

tests/test_docopt.py

        docopt("Usage: prog", "-x")

-    with raises(DocoptLanguageError):
+    # -o needs an argument in usage. The "unmatched" error is because `-o` is


Could you open a separate issue for this? I don't think I totally understand

Sure, I've taken out the message assertion/comment. It's a weird quirk of the way the parser is implemented, not at all intuitive. (Just to avoid any confusion, this isn't something I've changed here, it's just existing behaviour that I was asserting the message of.)

I've fixed the issue in a separate branch I'm working on to improve the usability of the error messages, it can wait for that.

NickCrews · 2022-09-07T23:18:40Z

This looks great @h4l, but let's wait for #36 to merge so we can then rebase on top of that?

codecov · 2022-09-07T23:20:33Z

Codecov Report

Merging #37 (c817b29) into master (850293b) will increase coverage by 0.20%.
The diff coverage is 100.00%.

❗ Current head c817b29 differs from pull request most recent head df66068. Consider uploading reports for the commit df66068 to get more accurate results

@@             Coverage Diff             @@
##           master       #37      +/-   ##
===========================================
+ Coverage   99.79%   100.00%   +0.20%     
===========================================
  Files           1         1              
  Lines         487       492       +5     
===========================================
+ Hits          486       492       +6     
+ Misses          1         0       -1

Impacted Files	Coverage Δ
docopt/__init__.py	`100.00% <100.00%> (+0.20%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

h4l · 2022-09-08T10:50:54Z

Yep, I can rebase this up once 36 is sorted.

NickCrews · 2022-09-08T21:22:02Z

Eh I just tried to fix the merge conflicts in the Web UI and it doesn't look that good, do you think you could actually do a rebase locally and fix the conflicts in their respective commits?

The .gitignore now ignores all dotfiles at the repo root, other than those explicitly not ignored with !.

Flake8 will spend a long time scanning the virtualenv files dir if the readme instructions are followed, creating one at /.venv.

It's a bit distracting when running tests during development to have coverage printed every time, and slows down the tests somewhat. Coverage and mypy are now run by CI, but not by default with pytest.

It was un-covered in the coverage report.

I need to know that these code paths are hit by tests.

These errors are not triggered by current tests.

This can only occur because of programmer error.

These errors are caused by bad usage docs, they shouldn't ever trigger DocoptExit as they're not the end user's fault.

A lot of docopt tests call docopt() without specifying argv, which uses `sys.argv` by default, so a predictable value for it is necessary.

I'm planning to make the error messages more user-friendly, (especially "Warning: found unmatched (duplicate?) arguments ..."), so I need to verify what the messages currently are so I can ensure that any changes are expected.

h4l · 2022-09-09T09:02:37Z

@NickCrews No problem, there were quite a few merge conflicts in there! I've rebased it on master.

Two of the changes/fixes from here were indirectly fixed by #36 so they're gone now:

Should be good to go!

NickCrews · 2022-09-09T17:43:41Z

Excellent work @h4l ! I'm going to add the isort-ing now. Didn't want to add before and cause more merge conflicts.

h4l · 2022-09-09T18:00:10Z

Very sensible, thanks @NickCrews! I guess when you do the isort pr it's probably easier to just take the change adding the isort config and re-run it to update the files rather than using the commit I made.

h4l force-pushed the test-error-messages branch from 1f166be to 93ea3f0 Compare August 28, 2022 09:25

h4l force-pushed the test-error-messages branch from 93ea3f0 to 9c0f189 Compare September 1, 2022 18:00

NickCrews reviewed Sep 7, 2022

View reviewed changes

tests/test_docopt.py Outdated Show resolved Hide resolved

NickCrews reviewed Sep 7, 2022

View reviewed changes

h4l force-pushed the test-error-messages branch from 51d3d1f to c817b29 Compare September 8, 2022 10:43

h4l added 12 commits September 8, 2022 21:44

chore: ignore dotfiles by default

03d35ce

The .gitignore now ignores all dotfiles at the repo root, other than those explicitly not ignored with !.

chore(build): configure flake8 to ignore hidden files/dirs

f2fb647

Flake8 will spend a long time scanning the virtualenv files dir if the readme instructions are followed, creating one at /.venv.

chore(build): don't auto-run coverage & mypy with pytest

cd846e7

It's a bit distracting when running tests during development to have coverage printed every time, and slows down the tests somewhat. Coverage and mypy are now run by CI, but not by default with pytest.

refactor: remove unnecessary future import

be60a28

test: test ParsedOptions repr

e56324c

It was un-covered in the coverage report.

test: remove pragma: no cover from exception raises

ca885c7

I need to know that these code paths are hit by tests.

test: add tests for parse_longer/_shorter errors

20cd25a

These errors are not triggered by current tests.

fix: report parse_longer() invalid arg with ValueError

45ea36a

This can only occur because of programmer error.

fix: always report duplicate options with DocoptLanguageError

998db81

These errors are caused by bad usage docs, they shouldn't ever trigger DocoptExit as they're not the end user's fault.

test: use a stable sys.argv value during tests

118f29e

A lot of docopt tests call docopt() without specifying argv, which uses `sys.argv` by default, so a predictable value for it is necessary.

test: assert the messages of raised errors

db47995

I'm planning to make the error messages more user-friendly, (especially "Warning: found unmatched (duplicate?) arguments ..."), so I need to verify what the messages currently are so I can ensure that any changes are expected.

test: remove obsolete Python 2 test

df66068

h4l force-pushed the test-error-messages branch from 85506a9 to df66068 Compare September 9, 2022 08:56

NickCrews merged commit 296ca67 into jazzband:master Sep 9, 2022

NickCrews mentioned this pull request Sep 9, 2022

Question: auto formatting & sorting imports #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assert error message content in tests #37

Assert error message content in tests #37

h4l commented Aug 28, 2022

h4l commented Aug 28, 2022

h4l commented Aug 28, 2022

h4l commented Sep 1, 2022 •

edited

Loading

NickCrews Sep 7, 2022 •

edited

Loading

h4l Sep 8, 2022

NickCrews commented Sep 7, 2022

codecov bot commented Sep 7, 2022 •

edited

Loading

h4l commented Sep 8, 2022

NickCrews commented Sep 8, 2022

h4l commented Sep 9, 2022 •

edited

Loading

NickCrews commented Sep 9, 2022

h4l commented Sep 9, 2022

Assert error message content in tests #37

Assert error message content in tests #37

Conversation

h4l commented Aug 28, 2022

h4l commented Aug 28, 2022

h4l commented Aug 28, 2022

h4l commented Sep 1, 2022 • edited Loading

NickCrews Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

h4l Sep 8, 2022

Choose a reason for hiding this comment

NickCrews commented Sep 7, 2022

codecov bot commented Sep 7, 2022 • edited Loading

Codecov Report

h4l commented Sep 8, 2022

NickCrews commented Sep 8, 2022

h4l commented Sep 9, 2022 • edited Loading

NickCrews commented Sep 9, 2022

h4l commented Sep 9, 2022

h4l commented Sep 1, 2022 •

edited

Loading

NickCrews Sep 7, 2022 •

edited

Loading

codecov bot commented Sep 7, 2022 •

edited

Loading

h4l commented Sep 9, 2022 •

edited

Loading