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

fix: default value not covered by parameters passed through feature file #737

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yunxuo
Copy link

@yunxuo yunxuo commented Nov 8, 2024

Describe the bug
when given multi description for step as below code

@given('a user with username')
@given(parsers.parse('a user with username {username}'))
@given(parsers.parse('a user with username {username} and password {password}'))
def create_user(user, username="defaultuser", password='defaultpassword'):
    user['username'] = username
    user['password'] = password

and pass parameters from feature file in Scenario Outline

Scenario Outline: User login
    Given a user with username <username> and password <password>
    When the user logs in with username <username> and password <password>
    Then the login should be <result>

    Examples:
      | username | password    | result  |
      | testuser | password123 | success |

Parameters username, password parameters passed through feature file cannot take effect.
The default values in def are always used。
in above case, the result is

user = {
    'username': 'defaultuser',
    'password': 'defaultpassword'
}

see also:
#610 pr fix this problems
#512

Copy link
Collaborator

@jsa34 jsa34 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Can we add a test for this?

@yunxuo
Copy link
Author

yunxuo commented Nov 14, 2024

Thanks for the PR! Can we add a test for this?

yes!

@yunxuo yunxuo requested a review from jsa34 November 15, 2024 02:45
Copy link
Collaborator

@jsa34 jsa34 left a comment

Choose a reason for hiding this comment

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

I cannot see any changes? I was expecting the tests to be added

@yunxuo
Copy link
Author

yunxuo commented Nov 15, 2024

I cannot see any changes? I was expecting the tests to be added

sorry I'm late.
I have pushed a test case in commit, but I don't know if it meets the repository specifications.

Copy link
Collaborator

@jsa34 jsa34 left a comment

Choose a reason for hiding this comment

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

I tried locally, and noticed tests fail - there seems to be issues with some step args now.

Could you address these test failures?

It appears to be when the kwarg isn't an arg of the step

@yunxuo
Copy link
Author

yunxuo commented Nov 15, 2024

I tried locally, and noticed tests fail - there seems to be issues with some step args now.

Could you address these test failures?

It appears to be when the kwarg needs to exist but isn't an arg of the step

image yes! when the kwarg needs to exist but isn't an arg of the step, so it will removed by following code

Do you mean the case I added failed?
in current pytest_bdd version, it will failed, but in this pr, will fix it

@jsa34
Copy link
Collaborator

jsa34 commented Nov 15, 2024

No, on the existing tests.

I will invoke a run to report the tests that fail

@jsa34
Copy link
Collaborator

jsa34 commented Nov 15, 2024

@jsa34
Copy link
Collaborator

jsa34 commented Nov 15, 2024

I took a look quickly to see if I could find a way to fix this and maintain the cases I mentioned, and came up with something like:

def _execute_step_function(
    request: FixtureRequest, scenario: Scenario, step: Step, context: StepFunctionContext
) -> None:
    """Execute step function."""
    __tracebackhide__ = True

    func_sig = signature(context.step_func)
    converters = context.converters

    def _get_parsed_arguments():
        """Parse and convert step arguments."""
        parsed_args = context.parser.parse_arguments(step.name)
        if parsed_args is None:
            raise ValueError(
                f"Unexpected `NoneType` returned from parse_arguments(...) in parser: {context.parser!r}"
            )
        kwargs = {}
        for arg, value in parsed_args.items():
            param = func_sig.parameters.get(arg)
            if param:
                if arg in converters:
                    value = converters[arg](value)
                kwargs[arg] = value
        return kwargs

    def _get_argument_values(kwargs):
        """Get default values or request fixture values for missing arguments."""
        for arg in get_args(context.step_func):
            if arg not in kwargs:
                param = func_sig.parameters.get(arg)
                if param:
                    if param.default != param.empty:
                        kwargs[arg] = param.default
                    else:
                        kwargs[arg] = request.getfixturevalue(arg)
        return kwargs

    kw = {
        "request": request,
        "feature": scenario.feature,
        "scenario": scenario,
        "step": step,
        "step_func": context.step_func,
        "step_func_args": {},
    }

    request.config.hook.pytest_bdd_before_step(**kw)

    try:
        # Use internal methods without passing redundant arguments
        kwargs = _get_parsed_arguments()

        if "datatable" in func_sig.parameters and step.datatable is not None:
            kwargs["datatable"] = step.datatable.raw()
        if "docstring" in func_sig.parameters and step.docstring is not None:
            kwargs["docstring"] = step.docstring

        kwargs = _get_argument_values(kwargs)

        kw["step_func_args"] = kwargs

        request.config.hook.pytest_bdd_before_step_call(**kw)

        return_value = call_fixture_func(fixturefunc=context.step_func, request=request, kwargs=kwargs)

    except Exception as exception:
        request.config.hook.pytest_bdd_step_error(exception=exception, **kw)
        raise

    if context.target_fixture is not None:
        inject_fixture(request, context.target_fixture, return_value)

    request.config.hook.pytest_bdd_after_step(**kw)

If this helps :)

@jsa34 jsa34 changed the title fix: defalut value not covered by parameters passed through feature file fix: default value not covered by parameters passed through feature file Nov 16, 2024
@yunxuo
Copy link
Author

yunxuo commented Nov 18, 2024

I took a look quickly to see if I could find a way to fix this and maintain the cases I mentioned, and came up with something like:

def _execute_step_function(
    request: FixtureRequest, scenario: Scenario, step: Step, context: StepFunctionContext
) -> None:
    """Execute step function."""
    __tracebackhide__ = True

    func_sig = signature(context.step_func)
    converters = context.converters

    def _get_parsed_arguments():
        """Parse and convert step arguments."""
        parsed_args = context.parser.parse_arguments(step.name)
        if parsed_args is None:
            raise ValueError(
                f"Unexpected `NoneType` returned from parse_arguments(...) in parser: {context.parser!r}"
            )
        kwargs = {}
        for arg, value in parsed_args.items():
            param = func_sig.parameters.get(arg)
            if param:
                if arg in converters:
                    value = converters[arg](value)
                kwargs[arg] = value
        return kwargs

    def _get_argument_values(kwargs):
        """Get default values or request fixture values for missing arguments."""
        for arg in get_args(context.step_func):
            if arg not in kwargs:
                param = func_sig.parameters.get(arg)
                if param:
                    if param.default != param.empty:
                        kwargs[arg] = param.default
                    else:
                        kwargs[arg] = request.getfixturevalue(arg)
        return kwargs

    kw = {
        "request": request,
        "feature": scenario.feature,
        "scenario": scenario,
        "step": step,
        "step_func": context.step_func,
        "step_func_args": {},
    }

    request.config.hook.pytest_bdd_before_step(**kw)

    try:
        # Use internal methods without passing redundant arguments
        kwargs = _get_parsed_arguments()

        if "datatable" in func_sig.parameters and step.datatable is not None:
            kwargs["datatable"] = step.datatable.raw()
        if "docstring" in func_sig.parameters and step.docstring is not None:
            kwargs["docstring"] = step.docstring

        kwargs = _get_argument_values(kwargs)

        kw["step_func_args"] = kwargs

        request.config.hook.pytest_bdd_before_step_call(**kw)

        return_value = call_fixture_func(fixturefunc=context.step_func, request=request, kwargs=kwargs)

    except Exception as exception:
        request.config.hook.pytest_bdd_step_error(exception=exception, **kw)
        raise

    if context.target_fixture is not None:
        inject_fixture(request, context.target_fixture, return_value)

    request.config.hook.pytest_bdd_after_step(**kw)

If this helps :)

Great code! it's worked in my case.
u can changed this pr

jsa34 and others added 3 commits November 19, 2024 19:03
Add regression tested code to not break when Args not in the method signature are present
Add type hints
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 84.84848% with 5 lines in your changes missing coverage. Please review.

Project coverage is 95.76%. Comparing base (93e446e) to head (b453257).

Files with missing lines Patch % Lines
src/pytest_bdd/scenario.py 82.14% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #737      +/-   ##
==========================================
- Coverage   95.95%   95.76%   -0.19%     
==========================================
  Files          55       55              
  Lines        2199     2220      +21     
  Branches      242      247       +5     
==========================================
+ Hits         2110     2126      +16     
- Misses         54       56       +2     
- Partials       35       38       +3     

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


🚨 Try these New Features:

@jsa34
Copy link
Collaborator

jsa34 commented Nov 19, 2024

@youtux are you happy with the codecov? If so, can we merge?

@jsa34 jsa34 requested a review from youtux November 19, 2024 21:27
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.

2 participants