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

Add hint to error message #35

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

Conversation

karlkovaciny
Copy link

I was playing with the tutorial at https://smarie.github.io/python-pytest-steps/#c-optional-steps-and-dependencies and got this error.

I had indented badly and my code looked like this:
with optional_step('step_b') as step_b:
assert True
yield step_b # should have been unindented, caused this error.

I just wanted to help any other newbie get past this in case it happened to them.

I was playing with the tutorial at https://smarie.github.io/python-pytest-steps/#c-optional-steps-and-dependencies and got this error.

I had indented badly and my code looked like this: 
    with optional_step('step_b') as step_b:
        assert True
        yield step_b # should have been unindented, caused this error.

I just wanted to help any other newbie get past this in case it happened to them.
@codecov-io
Copy link

codecov-io commented Nov 4, 2019

Codecov Report

Merging #35 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #35   +/-   ##
=======================================
  Coverage   83.52%   83.52%           
=======================================
  Files          27       27           
  Lines        1153     1153           
=======================================
  Hits          963      963           
  Misses        190      190
Impacted Files Coverage Δ
pytest_steps/steps_generator.py 79.55% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 575028b...00d708b. Read the comment docs.

@@ -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

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

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 :)

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.

None yet

3 participants