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

Move helper code for integration plugin #626

Closed
wants to merge 8 commits into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Mar 20, 2017

fd1b4f6 broke integration tests because the integration helper imports code from ``ipatests.test_integration```.

Traceback (most recent call last):
  File "/usr/bin/ipa-test-config", line 30, in <module>
    from ipatests.test_integration import config, env_config
  File "/usr/lib/python2.7/site-packages/ipatests/test_integration/__init__.py", line 22, in <module>
    if pytest.config.getoption('ipaclient_unittests', False):
AttributeError: 'module' object has no attribute 'config'

Helper code for ipatests.pytest_plugins.integration was in ipatests.test_integration. This doesn't play nice with pytests auto-discovery of test cases. Certain aspects of pytest are not available right away. For example pytest.config is generated after configuration stage but before discovery stage.

Now all helper code is next to the plugin in ipatests.pytest_plugins.integration (which is now a package).

https://pagure.io/freeipa/issue/6798

@pvoborni
Copy link
Member

From the PR description it is not clear what problem it solves or if it solves a problem.
"doesn't play nice " is vague.

" Certain aspects of pytest are not available right away. For example pytest.config is generated after configuration stage but before discovery stage." Is a description of reality, not a problem.

In other word. Why is this needed? And I'm not implying it is not needed, just the PR comment doesn't explain it.

Copy link
Contributor

@apophys apophys left a comment

Choose a reason for hiding this comment

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

LGTM with one issue I commented inline. The helper scripts work with these commits. Thanks for fixing the issue.

@@ -17,51 +17,11 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import time
import re

from ipaplatform.paths import paths
from ipalib.constants import DEFAULT_CONFIG

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are moving the modules into a package, why do we keep this module around? I do not know the rationale for the util module in here but the functions could move to ipatests/pytest_plugins/integration/tasks.py, with appropriate changes to test_legacy_clients, test_sudo and test_trust modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding to this, shouldn't we move the ipatests/test_integration/base.py module as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

base.py provides base class for all integration tests. The pytest plugin doesn't depend on the base class directly IMO it makes more sense to keep it next to the test cases.

For the other functions in module, I don't really care. If you prefer to have them in tasks, I'm happy to move them, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that the plugin itself does not depend on the integration test base class. As for the functions in the utils module, please move them to tasks.

@apophys
Copy link
Contributor

apophys commented Mar 21, 2017

Thanks for the update

@apophys apophys added the ack Pull Request approved, can be merged label Mar 21, 2017
@tiran
Copy link
Member Author

tiran commented Mar 22, 2017

@apophys Please create a ticket for the issue. This PR should not get merged until the issue is properly documented in a ticket and commit messages have been updated.

@apophys
Copy link
Contributor

apophys commented Mar 22, 2017

will do

The changes made to ipa-run-tests script in fd1b4f6 broke the
ipa-test-config and ipa-test-task scripts which are not executed
via pytest.

To fix the issue, all helper code and dependencies of the integration
plugin are moved out of ipatests.test_integration and into the
integration plugin. As first step ipatests.pytest_plugins.integration
is turned into a package.

https://pagure.io/freeipa/issue/6798
Signed-off-by: Christian Heimes <cheimes@redhat.com>
https://pagure.io/freeipa/issue/6798
Signed-off-by: Christian Heimes <cheimes@redhat.com>
https://pagure.io/freeipa/issue/6798
Signed-off-by: Christian Heimes <cheimes@redhat.com>
https://pagure.io/freeipa/issue/6798
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Copy link
Member Author

tiran commented Mar 22, 2017

@apophys has created ticket https://pagure.io/freeipa/issue/6798 and I have updated all commit messages.

@tkrizek tkrizek added the pushed Pull Request has already been pushed label Mar 22, 2017
@tkrizek
Copy link
Contributor

tkrizek commented Mar 22, 2017

master:

  • dde71ec Move helper code for integration plugin

  • 2895e39 Move config module to ipatests.pytest_plugins.integration.config

  • 1406dbc Move env_config module to ipatests.pytest_plugins.integration.env_config

  • 313ae46 Move tasks module to ipatests.pytest_plugins.integration.tasks

  • 8867412 Move hosts module to ipatests.pytest_plugins.integration.hosts

  • 8aadd55 Move function run_repeatedly to tasks module

  • 5587a37 Ship ipatests.pytest_plugins.integration

  • 24161a6 Move remaining util functions to tasks module
    ipa-4-5:

  • 1199416 Move helper code for integration plugin

  • 025a19c Move config module to ipatests.pytest_plugins.integration.config

  • e257bbd Move env_config module to ipatests.pytest_plugins.integration.env_config

  • 321437c Move tasks module to ipatests.pytest_plugins.integration.tasks

  • 6789dac Move hosts module to ipatests.pytest_plugins.integration.hosts

  • 4c62c41 Move function run_repeatedly to tasks module

  • 87b60f3 Ship ipatests.pytest_plugins.integration

  • cd79184 Move remaining util functions to tasks module

@tkrizek tkrizek closed this Mar 22, 2017
@tiran tiran deleted the test_integration branch April 4, 2017 08:12
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
4 participants