Add note on sharing the slow marker in the basic examples

[felipedau]

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]

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

[nicoddemus]

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]

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]

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

[felipedau]

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]

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

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]

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]

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

[felipedau]

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]

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

[nicoddemus]

Thanks a lot @felipedau!