Skip to content

Add hint to error message #35

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.

Sign up for GitHub

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

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

Add hint to error message #35

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pytest_steps/steps_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,8 @@ def execute(self, step_name, *args, **kwargs):
elif isinstance(res, optional_step):
# optional step: check if the execution went well
if res.exec_result is None:
raise ValueError("Internal error: this should not happen")
raise ValueError("Internal error: this should not happen."
"Did you ``yield step_b`` inside the context manager instead of after it?")
Copy link
Owner

@smarie smarie Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion. We just need to use the actual step name rather than "step_b", I guess this should do the trick, can you check ?

Suggested change
"Did you ``yield step_b`` inside the context manager instead of after it?")
"Did you ``yield %s`` inside the context manager instead of after it?" % optional_step.step_name)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this time optional_step is an instance of type, so you need to read res.step_name instead.

Suggested change
"Did you ``yield step_b`` inside the context manager instead of after it?")
raise ValueError("Internal error: this should not happen.\n"
"Did you ``yield %s`` inside the context manager instead of after it?" % res.step_name)

Here is a case that reproduces the error, do you want me to turn it into a test or something?

from pytest_steps import test_steps, optional_step

@test_steps('step_a')
def test_suite_opt():

    # Step A
    with optional_step('step_a') as step_a:
        assert True
        yield step_a

By the way, your efforts at documentation, code clarity, and response to pull requests are all just exemplary. Thanks a lot for supporting this.

Copy link
Owner

@smarie smarie Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops thanks for catching my mistake, I edited your PR too fast. Of course your suggestion is the correct one.

Concerning the test: I actually hesitated to ask you this ! but since you're proposing, yes do not hesitate to add such a test, in a separate test file under test/ (and not test_raw/) folder.

But I can also do it once merged based on your example, so do not feel forced, up to you.

Oh and thanks so much for the kind words ! I really appreciate :)


elif isinstance(res.exec_result, OptionalStepException):
# An exception happened in the optional step. We can now raise it safely
Expand Down