Various fixes to tests infrastructure #246

simo5 · 2019-07-10T14:49:52Z

Fix issues cropping up with newer python tools versions
Fixes #245
Fixes #247

simo5 · 2019-07-10T14:50:05Z

stanislavlevin

Hi, @simo5
You could use get_closest_marker.
For example,

-    if skip_servertest and item.get_marker("servertest") is not None:
+    # pytest 4+
+    if hasattr(item, 'get_closest_marker'):
+        get_marker = item.get_closest_marker
+    else:
+        get_marker = item.get_marker
+    if skip_servertest and get_marker("servertest") is not None:

simo5 · 2019-08-09T11:15:40Z

@stanislavlevin would you mind reviewing this PR given you proposed #248 and therefore are familiar with the issue ?

stanislavlevin · 2019-08-09T13:57:51Z

Please, let me check.

stanislavlevin

@simo5, hi! Please, see inline comments.

src/custodia/cli/__init__.py

src/custodia/ipa/interface.py

src/custodia/store/encgen.py

src/custodia/store/sqlite.py

tests/functional/base.py

tests/test_custodia.py

tox.ini

stanislavlevin

-    if skip_servertest and item.get_marker("servertest") is not None:
+    # pytest 4+
+    if hasattr(item, 'get_closest_marker'):
+        get_marker = item.get_closest_marker
+    else:
+        get_marker = item.get_marker
+    if skip_servertest and get_marker("servertest") is not None:

You could use a simpler version of fix with support of pytest < 4 .

simo5 · 2019-08-19T09:50:35Z

@stanislavlevin thanks for the review, sorry for late reply, I was away

simo5 · 2019-08-19T09:54:12Z

-    if skip_servertest and item.get_marker("servertest") is not None:
+    # pytest 4+
+    if hasattr(item, 'get_closest_marker'):
+        get_marker = item.get_closest_marker
+    else:
+        get_marker = item.get_marker
+    if skip_servertest and get_marker("servertest") is not None:

You could use a simpler version of fix with support of pytest < 4 .

Do you have a suggestion ?

stanislavlevin · 2019-08-19T09:57:07Z

-    if skip_servertest and item.get_marker("servertest") is not None:
+    # pytest 4+
+    if hasattr(item, 'get_closest_marker'):
+        get_marker = item.get_closest_marker
+    else:
+        get_marker = item.get_marker
+    if skip_servertest and get_marker("servertest") is not None:

You could use a simpler version of fix with support of pytest < 4 .

Do you have a suggestion ?

That diff is the suggestion ;-)

simo5

-    if skip_servertest and item.get_marker("servertest") is not None:
+    # pytest 4+
+    if hasattr(item, 'get_closest_marker'):
+        get_marker = item.get_closest_marker
+    else:
+        get_marker = item.get_marker
+    if skip_servertest and get_marker("servertest") is not None:

You could use a simpler version of fix with support of pytest < 4 .

Do you have a suggestion ?

Heh, nevermind, I just now realized this was the suggestion ... sometimes the github UI makes it confusing to distinguish actual code quoted from proposed code quoted ...

That said,
I am not sure get_closest_marker gives me what I want, I had considered it and decided to check for the specific marker.
But what I can do is retain the pytest < 4 code for backwards compat I guess.

make was broken in various ways on my F30 machine. - Some minor liniting issues - A connection reset by peer error instead of SSLv3 alert on some python versions - disabled python36-extras because it fails with a missing ipalib error - renamed all extrs/noextra to extras/noextras ... - fixed pytest deprecation warning error about using pytset.config Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2019-08-19T10:21:33Z

Ok rebased with the requested changes, modulo differences in how to handle the get_marker replacement

stanislavlevin · 2019-08-19T12:33:44Z

-    if skip_servertest and item.get_marker("servertest") is not None:
+    # pytest 4+
+    if hasattr(item, 'get_closest_marker'):
+        get_marker = item.get_closest_marker
+    else:
+        get_marker = item.get_marker
+    if skip_servertest and get_marker("servertest") is not None:
You could use a simpler version of fix with support of pytest < 4 .
Do you have a suggestion ?
Heh, nevermind, I just now realized this was the suggestion ... sometimes the github UI makes it confusing to distinguish actual code quoted from proposed code quoted ...

That said,
I am not sure get_closest_marker gives me what I want, I had considered it and decided to check for the specific marker.
But what I can do is retain the pytest < 4 code for backwards compat I guess.

Please, take a look at the official docs:
https://docs.pytest.org/en/latest/historical-notes.html#update-marker-code
http://doc.pytest.org/en/latest/example/markers.html#marking-whole-classes-or-modules

for marker in item.iter_markers():
    if marker.name == "servertest":
        # args has --skip-servertests and test is marked as servertest
        pytest.skip("Skip integration test")

The iteration is extra operation here, because iter_markers iterates over all markers of the item:
https://docs.pytest.org/en/latest/reference.html?highlight=get_closest_marker#_pytest.nodes.Node.iter_markers
Instead, you could filter marks with name=servertest, but that still returns generator of all the marks applied to the item (marks with the same name could be applied on test function, class or module levels). This can be done like:

if skip_servertest:
    if next(item.iter_markers("servertest"), None) is not None:
        # args has --skip-servertests and test is marked as servertest
        pytest.skip("Skip integration test")

or you could just use the syntax sugar (get_closest_marker).

Internally, get_closest_marker calls iter_markers(name=name) and returns the first mark from generator.
https://docs.pytest.org/en/latest/_modules/_pytest/nodes.html#Node.get_closest_marker

Honestly speaking, the custodia test suite has only 1 mark and only one level for this mark, so, it doesn't matter how to handle this one 😄 But...

stanislavlevin · 2019-08-19T12:48:30Z

Another one point, what is about Python3.7 at Travis?
Like this:
0001-py37.patch.txt

simo5 · 2019-08-19T16:06:26Z

Ok let me use get_closer_marker() and if later we have issues we'll fix em later...

As for py37 I think I tried to add it and something else blew up so I backed out :)

simo5 · 2019-08-19T16:13:18Z

Pushed, let's see what happens

simo5 · 2019-08-19T16:45:05Z

@stanislavlevin sounds like all checks pass, can you approve a review? (can't recall if permissions are set up that you can or not, let me know if you can't)

simo5 · 2019-08-19T16:50:00Z

Sigh realized now I had opened this PR from a origin branch ... then kept pushing changes to my personal branch ... oooh well..

codecov-io · 2019-08-19T17:01:22Z

Codecov Report

Merging #246 into master will increase coverage by 0.41%.
The diff coverage is 39.13%.

@@            Coverage Diff             @@
##           master     #246      +/-   ##
==========================================
+ Coverage   73.73%   74.14%   +0.41%     
==========================================
  Files          40       40              
  Lines        4401     4409       +8     
  Branches      447      453       +6     
==========================================
+ Hits         3245     3269      +24     
+ Misses        988      980       -8     
+ Partials      168      160       -8

Impacted Files	Coverage Δ
src/custodia/cli/__init__.py	`44.32% <0%> (+1.08%)`	⬆️
tests/functional/base.py	`86.36% <0%> (ø)`	⬆️
src/custodia/store/sqlite.py	`80.15% <0%> (ø)`	⬆️
src/custodia/ipa/interface.py	`80.48% <100%> (ø)`	⬆️
tests/conftest.py	`66.66% <12.5%> (-21.57%)`	⬇️
src/custodia/store/encgen.py	`82.81% <50%> (ø)`	⬆️
tests/test_custodia.py	`95.41% <66.66%> (+0.32%)`	⬆️
src/custodia/httpd/server.py	`24.92% <0%> (+0.87%)`	⬆️
src/custodia/server/config.py	`77.88% <0%> (+0.96%)`	⬆️
... and 4 more

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 e3a08ff...8a443ad. Read the comment docs.

stanislavlevin · 2019-08-19T17:47:22Z

I've checked this PR locally against Python 3.7.3.
tox py37 env is OK, unless --skip-servertests.

tox.py3 --sitepackages -e py37 -- --skip-servertests

platform linux -- Python 3.7.3, pytest-5.1.0, py-1.8.0, pluggy-0.12.0
cachedir: .tox/py37/.pytest_cache
rootdir: /usr/src/RPM/BUILD/test/custodia, inifile: tox.ini
collected 162 items                                                                           

tests/test_authenticators.py EE
tests/test_cli.py EEEEE
tests/test_custodia.py EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE
tests/test_httpd.py E
tests/test_ipa.py EEEEEEEEEEEEEEEEEEEEEEEE
tests/test_message_kem.py EEEE
tests/test_misc.py EEE
tests/test_plugins.py EEEE
tests/test_secrets.py EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE
tests/test_server.py EEEE
tests/test_store.py EEE
tests/test_store_sqlite.py EEEEEEEEEEE
tests/functional/test_basics.py EEE
tests/functional/test_container.py EEEEEEEEE
tests/functional/test_key.py EEEEEEEEEEE

=========================================== ERRORS ============================================
_______________________ ERROR at setup of TestAuthenticators.test_cred
...
>           elif item.get_closer_marker("servertest"):
E           AttributeError: 'TestCaseFunction' object has no attribute 'get_closer_marker'

The method should be 'get_closest_marker', not 'get_closer_marker' ;-)

get_marker has been removed in pytest 4, so replace it with their suggested work around. Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2019-08-20T07:33:18Z

Oh my ... should be fixed now

stanislavlevin · 2019-08-20T08:10:53Z

Pytest	With Servertests	Python 2.7.16	Python 3.7.3
3.10.1	Y	OK	OK
3.10.1	N	OK	OK
5.1.0	Y	OK	OK
5.1.0	N	OK	OK

stanislavlevin

All tests passed.

stanislavlevin · 2019-08-20T08:25:06Z

@stanislavlevin sounds like all checks pass, can you approve a review? (can't recall if permissions are set up that you can or not, let me know if you can't)

You could merge directly, aren't you?

simo5 · 2019-08-20T08:38:25Z

Done,
thanks a lot for the review!

stanislavlevin · 2019-08-20T08:48:16Z

Thank you too!

stanislavlevin reviewed Aug 9, 2019

View reviewed changes

This was referenced Aug 9, 2019

Fix Pytest 5 errors #248

Closed

Test suite fails against Pytest 5 #247

Closed

stanislavlevin reviewed Aug 12, 2019

View reviewed changes

simo5 commented Aug 19, 2019

View reviewed changes

simo5 force-pushed the testfixes branch from 6d5838a to 585db81 Compare August 19, 2019 16:49

simo5 and others added 2 commits August 20, 2019 03:32

Replace use of pytest get_marker

0d9e377

get_marker has been removed in pytest 4, so replace it with their suggested work around. Signed-off-by: Simo Sorce <simo@redhat.com>

Add support for testing with python 3.7 on travis

8a443ad

simo5 force-pushed the testfixes branch from 585db81 to 8a443ad Compare August 20, 2019 07:32

stanislavlevin approved these changes Aug 20, 2019

View reviewed changes

simo5 merged commit 242042d into master Aug 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes to tests infrastructure #246

Various fixes to tests infrastructure #246

simo5 commented Jul 10, 2019 •

edited

Loading

simo5 commented Jul 10, 2019

stanislavlevin left a comment

simo5 commented Aug 9, 2019

stanislavlevin commented Aug 9, 2019

stanislavlevin left a comment

stanislavlevin left a comment

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

stanislavlevin commented Aug 19, 2019

simo5 left a comment

simo5 commented Aug 19, 2019 •

edited

Loading

stanislavlevin commented Aug 19, 2019 •

edited

Loading

stanislavlevin commented Aug 19, 2019

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

codecov-io commented Aug 19, 2019 •

edited

Loading

stanislavlevin commented Aug 19, 2019

simo5 commented Aug 20, 2019

stanislavlevin commented Aug 20, 2019

stanislavlevin left a comment

stanislavlevin commented Aug 20, 2019

simo5 commented Aug 20, 2019

stanislavlevin commented Aug 20, 2019

Various fixes to tests infrastructure #246

Various fixes to tests infrastructure #246

Conversation

simo5 commented Jul 10, 2019 • edited Loading

simo5 commented Jul 10, 2019

stanislavlevin left a comment

Choose a reason for hiding this comment

simo5 commented Aug 9, 2019

stanislavlevin commented Aug 9, 2019

stanislavlevin left a comment

Choose a reason for hiding this comment

stanislavlevin left a comment

Choose a reason for hiding this comment

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

stanislavlevin commented Aug 19, 2019

simo5 left a comment

Choose a reason for hiding this comment

simo5 commented Aug 19, 2019 • edited Loading

stanislavlevin commented Aug 19, 2019 • edited Loading

stanislavlevin commented Aug 19, 2019

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

simo5 commented Aug 19, 2019

codecov-io commented Aug 19, 2019 • edited Loading

Codecov Report

stanislavlevin commented Aug 19, 2019

simo5 commented Aug 20, 2019

stanislavlevin commented Aug 20, 2019

stanislavlevin left a comment

Choose a reason for hiding this comment

stanislavlevin commented Aug 20, 2019

simo5 commented Aug 20, 2019

stanislavlevin commented Aug 20, 2019

simo5 commented Jul 10, 2019 •

edited

Loading

simo5 commented Aug 19, 2019 •

edited

Loading

stanislavlevin commented Aug 19, 2019 •

edited

Loading

codecov-io commented Aug 19, 2019 •

edited

Loading