BUG: Fix use with doctest + fixtures by larsoner · Pull Request #41 · smarie/python-pytest-harvest

larsoner · 2020-09-11T11:57:03Z

Fixes #42

On master using --doctest-modules with fixtures leads to:

pytest_harvest/results_session.py:167: in get_session_synthesis_dct
    param_dct = get_pytest_params(item)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

item = <DoctestItem pytest_harvest.plugin.dummy>

    def get_pytest_params(item):
        """ Returns a dictionary containing a pytest session item's parameters """
    
        if isinstance(item, _MinimalItem):
            # Our special _MinimalItem object - when xdist is used and worker states have been saved + restored
            return item.get_pytest_params()
        else:
            param_dct = OrderedDict()
>           for param_name in item.fixturenames:  # note: item.funcargnames gives the exact same list
E           AttributeError: 'DoctestItem' object has no attribute 'fixturenames'

and also

pytest_harvest/results_session.py:234: in <genexpr>
    filtered_items = tuple(item for item in session.items if _pytest_item_matches_filter(item, filterset))
pytest_harvest/results_session.py:327: in _pytest_item_matches_filter
    elif any(item_obj.__module__ == f for f in filterset):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

.0 = <set_iterator object at 0x7ffe00770140>

>   elif any(item_obj.__module__ == f for f in filterset):
E   AttributeError: 'NoneType' object has no attribute '__module__'

Added test that fails on master but passes here, along with dummy doctestable function
Adjusted META test count
Fixed code by adding a conditional that avoids trying to get param names from DoctestItem
Fixed code by adding a conditional for the item_obj is None case
Added pytest_harvest/_version.py to .gitignore (I assume this is useful?)
Removed some trailing whitespace (more PEP8-y; my editor did it automatically; can revert if preferred)

FWIW locally not all tests pass actually, but the same set that fail on this PR also fail on master.

codecov-commenter · 2020-09-11T12:04:11Z

Codecov Report

Merging #41 into master will decrease coverage by 0.18%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   88.63%   88.45%   -0.19%     
==========================================
  Files          31       31              
  Lines        1065     1074       +9     
==========================================
+ Hits          944      950       +6     
- Misses        121      124       +3

Impacted Files	Coverage Δ
pytest_harvest/plugin.py	`62.12% <50.00%> (-0.19%)`	⬇️
pytest_harvest/results_session.py	`82.32% <60.00%> (-0.58%)`	⬇️
...test_harvest/tests_raw/test_get_session_results.py	`96.21% <100.00%> (+0.05%)`	⬆️

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 00a713c...0a90b0a. Read the comment docs.

smarie · 2020-09-14T20:05:55Z

Thanks @larsoner ! I'll have a look tomorrow

smarie · 2020-09-15T16:27:54Z

pytest_harvest/plugin.py

+def doctestable():
+    """Do nothing, but have a doctest.
+
+    Examples
+    --------
+    >>> 1 + 1
+    2
+    """
+    return


Cant we have this doctest somewhere in the tests/ folder rather ? I'm a bit reluctant to leave it in the "main" part. Will it execute as a doctest if it is a dedicated test_xxx.py file ? I guess so

I don't know if --doctests works for things in tests/, but I'll give it a shot

ok. If it ends up being overly complex to move this to tests/, another option is simply to leave it here, but to add an underscore to the name to make it private (if doctest is happy with it).

pytest_harvest/results_session.py

smarie · 2020-09-15T16:35:00Z

pytest_harvest/tests_raw/test_get_session_results.py


+# For some reason, adding a monkeypatch will cause an extra failure for
+# DoctestItem, possibly because it's a setup/teardown
+def test_deal_with_doctest(dummy):


Can you please extend your comment by explaining what is the dummy fixture here ? Maybe it is just a fixture provided by the monkeypatch plugin but I did not find any reference to it..

smarie · 2020-09-15T16:41:07Z

Thanks @larsoner ! Looks very good to me, I made a few comments that are mostly nitpicks really, just for the sake of making maintenance a bit easier on the long run.

All tests pass ok in Travis so I'll be able to merge when you'll have made the updates.
(the only failure that I see is in the "raw" tests, and is intended - it is then collected by the META test runner and expected to fail)

larsoner · 2020-09-16T13:54:41Z

@smarie comments addressed

setup.py

larsoner · 2020-09-18T16:30:04Z

There is a wall of red here :)

https://travis-ci.org/github/smarie/python-pytest-harvest/builds/727704884

Let me know if you want my help fixing it

smarie · 2020-09-20T14:00:54Z

Hi @larsoner I merged manually and fixed the meta-tester, everything is fine now and 1.9.3 is available.

There were 2 issues

first pytest starting from version 6 changed the order in which the parameters are returned by the internal API I was using
the meta-tests running sandboxed executions of pytest were not taking into account the configuration in setup.cfg, I had to generate a pytest.ini file to set the doctest option on.
(see https://docs.pytest.org/en/6.0.2/writing_plugins.html#testing-plugins)

Let me know if the released version is ok for you.
And thanks again !!

larsoner · 2020-09-21T18:03:17Z

@smarie yes, just pip install pytest-harvest works now!

smarie · 2020-09-22T07:09:04Z

great, thanks for the feedback and PR @larsoner !

BUG: Fix use with doctest + fixtures

0065f9d

larsoner mentioned this pull request Sep 11, 2020

MAINT: Show test timings mne-tools/mne-python#8240

Merged

smarie reviewed Sep 15, 2020

View reviewed changes

pytest_harvest/results_session.py Show resolved Hide resolved

smarie reviewed Sep 15, 2020

View reviewed changes

pytest_harvest/results_session.py Show resolved Hide resolved

smarie reviewed Sep 15, 2020

View reviewed changes

larsoner added 2 commits September 16, 2020 09:53

STY: Document and move

9289367

FIX: Revert

0a90b0a

smarie reviewed Sep 18, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

smarie merged commit 0a90b0a into smarie:master Sep 18, 2020

larsoner deleted the doctest branch September 21, 2020 11:05

Conversation

larsoner commented Sep 11, 2020 • edited by smarie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

smarie commented Sep 14, 2020

Uh oh!

smarie Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

smarie Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smarie Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

smarie commented Sep 15, 2020

Uh oh!

larsoner commented Sep 16, 2020

Uh oh!

Uh oh!

larsoner commented Sep 18, 2020

Uh oh!

smarie commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Sep 21, 2020

Uh oh!

smarie commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

larsoner commented Sep 11, 2020 •

edited by smarie

Loading

codecov-commenter commented Sep 11, 2020 •

edited

Loading

smarie Sep 15, 2020 •

edited

Loading

smarie commented Sep 20, 2020 •

edited

Loading