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 note on sharing the `slow` marker in the basic examples #2653

Merged
merged 2 commits into from Aug 9, 2017

Conversation

Projects
None yet
4 participants
@felipedau
Contributor

felipedau commented Aug 3, 2017

Hi! I recently faced the same issue as in #472 when trying to reuse the slow marker and it took me a while to understand why that was failing. I figured that having this note would be useful. If someone agrees, I can go through the checklist to make it a proper PR.

Question: in a similar issue (#1688) @nicoddemus said that "using pytest.config is discouraged". Is its use discouraged in that specific case or there is a better way to use the configs in general?

Thanks!


Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS;
@coveralls

This comment has been minimized.

coveralls commented Aug 3, 2017

Coverage Status

Coverage remained the same at 91.807% when pulling 916d137 on felipedau:slow-sharing-note into 8969bd4 on pytest-dev:master.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 3, 2017

Hi @felipedau, thanks a lot for the PR.

Looking at the code, I think it would be better to change the example itself to define the marker in a separate module (other than a conftest.py) instead of adding a note. What do you think?

I also wonder if we should advertise using pytest.config like the example shows... @RonnyPfannschmidt do we have concrete plans to ever remove/deprecate pytest.config, or can we advertise it without problems?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 4, 2017

Alternatively we can rewrite the example to use a pytest_collect_modifyitems() hook to look for the slow mark on the items and add the skipif mark accordingly. This avoids the pitfall altogether.

@RonnyPfannschmidt @hackebrot opinions?

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Aug 4, 2017

@nicoddemus we should finalize a plan for removal, its a bad hack

@felipedau

This comment has been minimized.

Contributor

felipedau commented Aug 4, 2017

Maybe it's because I am relatively new to pytest (well, to testing in general), but importing from another test module looks weird to me.

Alternatively we can rewrite the example to use a pytest_collect_modifyitems() hook to look for the slow mark on the items and add the skipif mark accordingly. This avoids the pitfall altogether.

I think it sounds a lot more elegant :)

@felipedau

This comment has been minimized.

Contributor

felipedau commented Aug 8, 2017

@nicoddemus, I made the changes you suggested in the project I am currently working on and I think it looks a lot better! (AnemoneLabs/unmessage@f1a61fc)

The problem is that it's still using pytest.config to check for the option. If I correctly understood @RonnyPfannschmidt, that should be removed. Is there another way to do it?

I do not think we are talking just about a note here. Should I close this PR?

Thanks!

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 9, 2017

Hi @felipedau, sorry for not getting back to you sooner.

I think we can rewrite this example from the docs: it is about how to skip tests based on a command line argument, not about using the skipif marker in particular.

Your version looks good, but I would simplify it a little bit like this:

def pytest_addoption(parser):
    parser.addoption('--runslow', action='store_true', 
                     default=False, help="run slow tests")

def pytest_collection_modifyitems(config, items):    
    if config.getoption('--runslow'):
        # --runslow given in cli: do not skip slow tests
        return
    skip_slow = pytest.mark.skip(reason='need --runslow option to run')
    for item in items:
        if 'slow' in item.keywords:
            item.add_marker(skip_slow)

Would you like to update the documentation to use this recipe?

@felipedau

This comment has been minimized.

Contributor

felipedau commented Aug 9, 2017

Hi @felipedau, sorry for not getting back to you sooner.

Don't worry. Not a problem at all :)

Would you like to update the documentation to use this recipe?

Thanks, it is indeed a lot simpler. I just made the update. I can fix/squash things, but I cannot rename the branch (otherwise it will close this PR). Is this okay or should I create a new one?

Thanks!

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 9, 2017

Thanks! Feel free to rebase/squash. I will review it first thing in the morning. 👍

@felipedau

This comment has been minimized.

Contributor

felipedau commented Aug 9, 2017

Thanks! Feel free to rebase/squash. I will review it first thing in the morning. 👍

Alright! Thanks @nicoddemus, I learned new things with your sugggestions :)

@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2017

Coverage Status

Coverage remained the same at 91.805% when pulling acd3c4f on felipedau:slow-sharing-note into 523bfa6 on pytest-dev:master.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 9, 2017

Thanks a lot @felipedau!

@nicoddemus nicoddemus merged commit e602078 into pytest-dev:master Aug 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@felipedau felipedau deleted the felipedau:slow-sharing-note branch Aug 9, 2017

fgmacedo added a commit to fgmacedo/pytest that referenced this pull request Sep 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment