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

Add options to run only ipaclient unittests #475

Closed
wants to merge 1 commit into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Feb 17, 2017

A new option for ipa-run-tests makes the test runner ignore
subdirectories or skips tests that depend on the ipaserver package or on
a running framework for RPC integration tests. The new option enables
testing of client-only builds.

$ ipatests/ipa-run-tests --ipaclient-unittests
...
platform linux2 -- Python 2.7.13, pytest-2.9.2, py-1.4.32, pluggy-0.3.1
rootdir: /home/heimes/redhat, inifile: tox.ini
plugins: sourceorder-0.5, cov-2.3.0, betamax-0.7.1, multihost-1.1
collected 451 items

test_util.py ........
util.py ..
test_ipaclient/test_csrgen.py ..............ssss...
test_ipalib/test_aci.py ...................
test_ipalib/test_backend.py ........
test_ipalib/test_base.py ...............
test_ipalib/test_capabilities.py .
test_ipalib/test_cli.py ...
test_ipalib/test_config.py ...............
test_ipalib/test_crud.py ...............
test_ipalib/test_errors.py .......
test_ipalib/test_frontend.py ........................................
test_ipalib/test_messages.py ....
test_ipalib/test_output.py ...
test_ipalib/test_parameters.py .............................................................
test_ipalib/test_plugable.py ........
test_ipalib/test_rpc.py ......ssssssss
test_ipalib/test_text.py .............................
test_ipalib/test_x509.py ...
test_ipapython/test_cookie.py ............
test_ipapython/test_dn.py ...........................
test_ipapython/test_ipautil.py ..................................................................
test_ipapython/test_ipavalidate.py ..........
test_ipapython/test_kerberos.py ..............
test_ipapython/test_keyring.py ..........
test_ipapython/test_ssh.py ...............................
test_pkcs10/test_pkcs10.py .....

https://fedorahosted.org/freeipa/ticket/6517

Signed-off-by: Christian Heimes cheimes@redhat.com

@tiran
Copy link
Member Author

tiran commented Feb 17, 2017

PS: I'm not attached to the new of the option. Please speak up if you can come up with a better name than --ipaclient-unittests.

@martbab
Copy link
Contributor

martbab commented Feb 17, 2017

I was thinking that instead of making up more options to test runner we could reorganize the ipatests/ directory to actually make sense from the consumer's POV, although I admit that it will take more time and also has potential to break are incredibly... fragile test handling.

On the plus side, you would run the tests you want naturally by just specifying the path that interests you and let the test discovery do the rest.

A silly example:

$ ipa-run-tests test_ipaclient/test_units
test_ipaclient/test_units/test_util.py ........
test_ipaclient/test_units/tutil.py ..
test_ipaclient/test_units/test_csrgen.py ..............ssss...
...

@tiran
Copy link
Member Author

tiran commented Feb 17, 2017

My first PoC used the approach. ipa-run-tests can already takes arguments to limit tests to subdirectories. One has to remember that ipa-run-tests performs chdir()...

The additional option combined with the marker have some benefits. It's not just less work and more robust, it permits us to use a cleaner and declarative way to mark test cases. The author of a test case can mark a test with pytest's standard API instead of changing some test runner options. The markers allow us to skip some test cases in a file, too. In my and your example, some tests of test_csrgen are skipped in --ipaclient-unittest mode. :)

@martbab
Copy link
Contributor

martbab commented Feb 28, 2017

I am not a big fan of mixing filename matching and markers in this PR. I feel that using only one of those approaches is a more cleaner solution and it seems that marking all the tests and then running a subset using the pytest's marker selection API loks like the easiest road.

It seems like a daunting task but it may actually be easier given that you can mark whole modules[1] or even generate marker dynamically by introspecting node IDs during test collection[2].

You can ultimately provide an option as an alias for selecting/deselecting markers as needed if you like but the underlying implementation will be cleaner as result.

[1] http://doc.pytest.org/en/latest/example/markers.html#marking-whole-classes-or-modules
[2] http://doc.pytest.org/en/latest/example/markers.html#automatically-adding-markers-based-on-test-names

@tiran
Copy link
Member Author

tiran commented Feb 28, 2017

I'm not a big fan either. Can you come up with a better solution that does not result in import errors? Because the module marker or class markers still import the whole module. For client-only tests, ipaserver is not available. For Python packaging builds, neither ipaserver nor ipaplatform are available.

@martbab
Copy link
Contributor

martbab commented Feb 28, 2017

Oh my, every time I think about something nice that should work there is some corner case that ruins it.

I guess that one way to work around it would be to keep the try: ... except importError guards in the offending modules and add skip markers like @pytest.mark.skipif(ipaserver is None, "ipaserver module unavailable") or skip whole modules.

As a side note, I really wish that our test suite would be a little less... um, special.

@tiran
Copy link
Member Author

tiran commented Feb 28, 2017

I pushed an alternative approach that checks for the option and raises skip in packages. It needs some extra workaround in the integration plugin.

@martbab
Copy link
Contributor

martbab commented Mar 3, 2017

I like the second approach better. If you squash the commits I will Ack the PR. I still think we need a substantial reorganization of the test suites but that needs more consideration and time.

@tiran
Copy link
Member Author

tiran commented Mar 14, 2017

@martbab I've cleanup up some white space noise and squashed all commits.

@tiran tiran mentioned this pull request Mar 15, 2017
5 tasks
A new option for ipa-run-tests makes the test runner ignore
subdirectories or skips tests that depend on the ipaserver package or on
a running framework for RPC integration tests. The new option enables
testing of client-only builds.

$ ipatests/ipa-run-tests --ipaclient-unittests
...
platform linux2 -- Python 2.7.13, pytest-2.9.2, py-1.4.32, pluggy-0.3.1
rootdir: /home/heimes/redhat, inifile: tox.ini
plugins: sourceorder-0.5, cov-2.3.0, betamax-0.7.1, multihost-1.1
collected 451 items

test_util.py ........
util.py ..
test_ipaclient/test_csrgen.py ..............ssss...
test_ipalib/test_aci.py ...................
test_ipalib/test_backend.py ........
test_ipalib/test_base.py ...............
test_ipalib/test_capabilities.py .
test_ipalib/test_cli.py ...
test_ipalib/test_config.py ...............
test_ipalib/test_crud.py ...............
test_ipalib/test_errors.py .......
test_ipalib/test_frontend.py ........................................
test_ipalib/test_messages.py ....
test_ipalib/test_output.py ...
test_ipalib/test_parameters.py .............................................................
test_ipalib/test_plugable.py ........
test_ipalib/test_rpc.py ......ssssssss
test_ipalib/test_text.py .............................
test_ipalib/test_x509.py ...
test_ipapython/test_cookie.py ............
test_ipapython/test_dn.py ...........................
test_ipapython/test_ipautil.py ..................................................................
test_ipapython/test_ipavalidate.py ..........
test_ipapython/test_kerberos.py ..............
test_ipapython/test_keyring.py ..........
test_ipapython/test_ssh.py ...............................
test_pkcs10/test_pkcs10.py .....

https://fedorahosted.org/freeipa/ticket/6517

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@@ -30,9 +30,6 @@

from ipapython import ipautil
from ipapython.ipa_log_manager import log_mgr
from ipatests.test_integration import tasks
from ipatests.test_integration.config import Config
from ipatests.test_integration.env_config import get_global_config

Copy link
Contributor

Choose a reason for hiding this comment

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

I fail to see why this change is needed can you please explain it to me?

Copy link
Member Author

Choose a reason for hiding this comment

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

tl;dr it breaks :)

It's a chicken and egg problem. ipatests.test_integration.__init__ checks for pytest.config.getoption('ipaclient_unittests', False). On the other hand, pytests imports the plugin when it loads conftest.py. At import time of conftest, pytest.config is not available. The config namespace is created at a later stage.

We should clean this up at a later stage and move all dependencies of the integration plugin out of test_integration. I tried it but it is a lot of work and requires heavy modifications of all integration tests. This approach is simpler and less risky.

@martbab
Copy link
Contributor

martbab commented Mar 17, 2017

I have one small question and am going to try out some integration tests to see if we did not break something in them as Travis won't catch that.

@martbab martbab added the ack Pull Request approved, can be merged label Mar 17, 2017
@martbab
Copy link
Contributor

martbab commented Mar 17, 2017

master:

  • fd1b4f6 Add options to run only ipaclient unittests
    ipa-4-5:

  • 29b885a Add options to run only ipaclient unittests

@martbab martbab added the pushed Pull Request has already been pushed label Mar 17, 2017
@martbab martbab closed this Mar 17, 2017
@tiran tiran deleted the ipaclient_unittests branch March 18, 2017 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants