Allow pulp-smash tests to live in pulp/pulp_ansible repo
RCMariko opened this issue · 18 comments
- Enable a pulp_ansible plugin contributor to send pulp-smash tests in the same PR as a feature.
- A specific folder location for all pulp-smash tests seems logical.
I acknowledge this would mean ownership of pulp-smash tests for the ansible plugin would at this time be owned by that plugin team.
Completion date of April 26 would allow developer to invite contribution of pulp-smash tests when new functionality is demo-ed.
What's the motivation for this request? It is that sending one pull request is (arguably) easier than sending two pull requests, or are there other reasons? Does this request apply to all plugins, not just pulp_ansible? Have the negative consequences been thought through? (There are negative consequences.) Have the other implementation options been though through? (e.g. namespace packages.)
This request has major consequences, and I need answers to at least these questions before moving forward.
The scope of this request is just the tests for this single plugin. I would like to pave the way for requiring community contributions for this plugin be delivered with both unit and functional tests without a lot of manual or automated checks.
I would welcome hearing of negative consequences.
I will work with @Ichimonji10 to get more constructive feedback to his original reply and post it here.
Typing one up now.
@Ichimonji10 start by checking my email?
@RCMariko Thanks for clarifying. The goal seems to be to streamline the process for running and implementing integration tests, so that the bar for contributions can be set higher. That's a nice goal. However, there are some significant challenges and drawbacks to the proposal as written. I've listed a few below. Perhaps it is possible to come up with some other techniques for streamlining things that avoids the challenges and drawbacks I've identified. I'd love to brainstorm and explore the problem space. Perhaps some git-subdir, git-subtree, git-submodule, or make magic could suffice.
I would welcome hearing of negative consequences.
OK. Let me give a familiar scenario. Let's say that I have a host on which I've installed Pulp 3 from PyPI or RPM packages, and let's say that I'd like to run integration tests against it. How would I do that? The current approach is as folows:
- Create a Python 3.4+ virtualenv.
- Install Pulp Smash. (Either from source or from PyPI.)
- Configure Pulp Smash. (e.g.
python -m pulp_smash settings create) - Run the tests. (e.g.
python -m unittest discover pulp_smash.tests)
What happens if this issue is implemented, and integration tests for pulp_ansible are moved into the pulp_ansible repository? Tests for pulp_ansible won't run. This is because pip install pulp-smash (or whatever equivalent was used at install time) won't pull in the pulp_ansible integration tests.
"Ah, but I can just install both Pulp Smash and pulp_ansible into the same virtualenv, and let the former discover the latter's namespace packages!"
Will that work? Here's some of the issues that need to be overcome:
- Pulp Smash and pulp_ansible must be installed on the same host, and pulp_ansible must be installed into a standard search path. Both of these requirements are problematic:
- Pulp Smash and pulp_ansible are frequently present on different hosts. This
isisn't the case on our current Jenkins testing infrastructure, but it's only by coincidence. The push to drop nodepool from Jenkins might invalidate this assumption. Implementation of multi-host testing would certainly invalidate this assumption. This assumption is already invalidated for me, as I have one Pulp Smash installation for many Pulp VMs. - The Pulp 3 installer (both from-source and from-PyPI) install Pulp and its plugins into non-standard paths.
- Pulp Smash and pulp_ansible are frequently present on different hosts. This
- Pulp Smash would need to be re-designed to support namespace packages. One of the big problems with namespace packages is namespace conflicts, where a name in a newly imported package conflicts with a name in the base application or one of the other namespace packages.
- Pulp Smash has no stable API. Code is periodically moved around or changed internally, sometimes in major ways. If third parties were to integrate with Pulp Smash, a stable API would need to be implemented. At the very least, this implies paying off some technical debt (e.g. moving more stuff out of
pulp_smash.*intopulp_smash.tests.pulp2.*). And implementing a stable API means Pulp Smash can't innovate and improve like it can now. - Splitting code across repositories means that the linters and unit tests built in to Pulp Smash won't be able to work their magic on the separated code. (It may still be doable with some technical magic.)
- Pulp Smash is a non-trivial application. It's reasonable to think that its requirements and pulp_ansible's requirements might conflict, meaning that installing them into the same virtualenv won't work.
Thanks @Ichimonji10 for the feedback. I think your concerns are very real and something we want to solve before we go down this path of allowing pulp-smash tests live inside plugins. Some thoughts below.
One question I have is: does Pulp QE want to maintain smash tests for all plugins? With Pulp 3 I think we'll see more community-contributed plugins like pulp_gem. If plugins start writing smash tests with our current model, PulpQE is going to need to review these PRs, maintain their code, etc. Seems like a better model would be to put the onus on plugins to manage their own smash tests.
Pulp Smash and pulp_ansible are frequently present on different hosts.
What if in this case we install two copies of pulp_ansible—one on each host?
The Pulp 3 installer (both from-source and from-PyPI) install Pulp and its plugins into non-standard paths.
Yea, we would need to deal with this.
Pulp Smash would need to be re-designed to support namespace packages.
Doesn't Pulp Smash already namespace plugin tests? Also, I think this responsibility would fall on plugins if they want to write pulp-smash tests so not sure if it's something QE needs to worry about?
Pulp Smash has no stable API.
What about a semantically versioned API? This might be a good compromise between stability and the freedom for pulp-smash to innovate/improve.
Splitting code across repositories means that the linters and unit tests built in to Pulp Smash won't be able to work their magic on the separated code.
In our PRs, we install pulp-smash into Travis and we could certainly run any linters or unit tests on any code changes to the pulp-smash tests inside pulp_ansible or whatever plugin. The reverse (adding new linters, etc) in pulp-smash could be a pain though.
Pulp Smash is a non-trivial application. It's reasonable to think that its requirements and pulp_ansible's requirements might conflict, meaning that installing them into the same virtualenv won't work.
Yes, agreed. This is something we'll have to solve although I think running pulp-smash tests against each PR would hopefully reveal such problems as they arise.
@kersommoura, feel free to chime in. Your opinion is important, especially on matters that have a long term impact on Pulp QE.
One question I have is: does Pulp QE want to maintain smash tests for all plugins? With Pulp 3 I think we'll see more community-contributed plugins like pulp_gem. If plugins start writing smash tests with our current model, PulpQE is going to need to review these PRs, maintain their code, etc. Seems like a better model would be to put the onus on plugins to manage their own smash tests.
I think it's valuable for Pulp QE to be able to write tests for any of the plugins for Pulp 3. However, Pulp QE is a small team, and it seems highly unlikely that we'll be able to thorougly test all plugins. This is especially true if a greater variety of plugins crops up for Pulp 3 than is currently the case for Pulp 2. I see great value in designing a mechanism by which third parties can write integration tests without having to re-invent the wheel, and without the frustration of merging all of those tests into Pulp Smash proper. The bar for contributions is set high, and I'd like to keep that bar high, despite the drawbacks of doing so.
What if in this case we install two copies of pulp_ansible—one on each host?
This seems like a viable solution. It does mean a little bit of coordination, where both the Pulp 3 installation and the Pulp Smash installation need to be kept in sync. But the burden seems manageable, and it allows Pulp 3 and Pulp Smash to be installed on separate hosts.
The Pulp 3 installer (both from-source and from-PyPI) install Pulp and its plugins into non-standard paths.
Yea, we would need to deal with this.
If two copies of pulp_ansible are installed as outlined above, this should no longer be an issue. Cool.
Doesn't Pulp Smash already namespace plugin tests? Also, I think this responsibility would fall on plugins if they want to write pulp-smash tests so not sure if it's something QE needs to worry about?
Like any Python application, Pulp Smash uses namespaces. However, it does not support namespace packages. Explicit work needs to be done to enable namespace packages.
I do think that Pulp QE needs to worry about namespace conflicts. Imagine the following scenario:
- The pulp_ansible plugin implements a test case called
pulp_smash.tests.pulp3.ansible.api_v3.test_role.RoleTestCase. - Pulp QE writes a test case at the same path.
Whoops. Also, I don't want third-party test writers to have to know about every going-on in Pulp Smash. That seems like an unnecessary burden. Thankfully, the design of namespace packages naturally encourages namespace separation, so this shouldn't be an issue.
What about a semantically versioned API? This might be a good compromise between stability and the freedom for pulp-smash to innovate/improve.
This is doable. There are projects that both have regular releases and maintain good semantic versioning. As an example, see Syncthing releases, which come out on a weekly basis. Some technical debt needs to be paid off, though. Some todos that come to mind are as follows:
- The
BaseAPITestCaseshould be deleted. - Much code should be moved out of
pulp_smash.*intopulp_smash.tests.pulp2.*andpulp_smash.tests.pulp3.*. - It'd probably be good to go through all existing class/method/function/constant/etc names with a fine-toothed comb and assign a good naming scheme.
If code is to get out the door sooner rather than later, we can create a mechanism for plug-in tests, but avoid an X release until this is done.
Splitting code across repositories means that the linters and unit tests built in to Pulp Smash won't be able to work their magic on the separated code.
In our PRs, we install pulp-smash into Travis and we could certainly run any linters or unit tests on any code changes to the pulp-smash tests inside pulp_ansible or whatever plugin. The reverse (adding new linters, etc) in pulp-smash could be a pain though.
Perhaps the solution here is to say "different projects have different standards." To hold every plugin to the same standard could easily be problematic. Could 1e9c99d and 89ca344 have been implemented if Pulp Smash had to worry about the linting status of every plugin? I don't think so.
Pulp Smash is a non-trivial application. It's reasonable to think that its requirements and pulp_ansible's requirements might conflict, meaning that installing them into the same virtualenv won't work.
Yes, agreed. This is something we'll have to solve although I think running pulp-smash tests against each PR would hopefully reveal such problems as they arise.
Perhaps the solution for this issue is to just cross those bridges when we come to them. There's significant benefit in allowing third parties to independently write tests, and it justifies some extra work. Also, I'd like to see one or two dependencies (plumbum!) dropped from Pulp Smash. This is a good motivating reason for accelerating that work.
This is a very interesting topic, however I have few doubts.
1 - Is there already any quality policy for third party plugins?
Are we suggesting the use of unit tests? In my opinion we should advocated for unit tests first, then suggest the addition of Pulp-Smash tests as an extra step. Avoid mixing these two concepts. They have different purposes.
2 - How are we going to enforce tests of certain features to be tested?
3 - As far as I know, ansible plugin will created and maintained by the Pulp team, why treat this one different from the others?
For Pulp2, we have a contribution coming from the community, in each one the contributor submitted a fix for the RPM plugin, and a Smash Test. Perhaps, if we can expand the Smash contribution section - Add more examples, and make life for the first time contributor a little bit easier, keeping the code standards and so on. This can beneficial for all plugins, and attempt to bring more contributions to Pulp-Smash before creating different name spaces could be an easier solution.
I agree with @Ichimonji10 that QE will not be able to test all third party plugins, and think about a feasible solution for them right now sounds good.
Are we suggesting the use of unit tests? In my opinion we should advocated for unit tests first, then suggest the addition of Pulp-Smash tests as an extra step. Avoid mixing these two concepts. They have different purposes.
There's great value in unit tests. They can be executed without spinning up a working Pulp application, and they can be several orders of magnitude faster than integration tests.
As far as I know, ansible plugin will created and maintained by the Pulp team, why treat this one different from the others?
If we go to the trouble of creating a mechanism by which third parties can independently write pulp_ansible integration tests, I'd like to make sure that it works for all plugins. One-off solutions to a general problem are a red flag.
1 - Is there already any quality policy for third party plugins?
Are we suggesting the use of unit tests? In my opinion we should advocated for unit tests first, then suggest the addition of Pulp-Smash tests as an extra step. Avoid mixing these two concepts. They have different purposes.
Agreed. Adding pulp_smash tests to pulp_ansible isn't a replacement for unit tests.
2 - How are we going to enforce tests of certain features to be tested?
My thinking is that we can open PRs against community plugins if we need certain features to have smash tests. I can't think why plugins wouldn't accept these PRs. Certainly on pulp_ansible, we'd welcome any contributions from QE. :)
If we go to the trouble of creating a mechanism by which third parties can independently write pulp_ansible integration tests, I'd like to make sure that it works for all plugins. One-off solutions to a general problem are a red flag.
Agreed. I'm imagining that perhaps one day all plugin smash tests could reside within their respective plugins. For certain plugins like pulp_rpm we could have QE team members be involved with reviewing PRs that touch pulp-smash tests. We could even leverage Github's code owner feature to make sure PRs that touch smash tests aren't merged without QE approval.
Perhaps, if we can expand the Smash contribution section - Add more examples, and make life for the first time contributor a little bit easier, keeping the code standards and so on. This can beneficial for all plugins, and attempt to bring more contributions to Pulp-Smash before creating different name spaces could be an easier solution.
I think this is worth considering too. A few downsides I see though: you can't submit updates to smash tests in the same PR, QE becomes responsible for reviewing/maintaining/etc smash tests for community plugins, and I still think no matter how easy we make it the barrier to entry is still going to be a bit higher than just having the smash tests in the same repo.
Thank you, @Ichimonji10 for providing detailed concerns regarding the suggestion. My desire to tackle this for just the pulp_ansible repo was purely due to wanting to come to consensus quickly on an incremental problem rather than trying to come up with a solution for a more complicated problem. It would also reduce the risk of making a change and also make a change with the most risk tolerant part (pulp_ansible has zero tests now and we aren't asking QE to support/own this now.) We could make the change for a plugin and we could learn from the experience rather than trying to come up with a solution for all plugins and take a longer time to learn what doesn't work and then make a change across all plugins. I see several suggestions here that also accomplish that.
I am fully supportive of us making progress on encouraging pulp-smash contributions on all plugins and coming up with incremental changes that are consistent across plugins (to the degree that they are consistent.) Maybe all productized plugins vs non-productized plugins? I advocate for as simple as possible and only making something more complicated if necessary.
I felt the demo of a new plugin with a call for integration test cases contributions may be helpful since users of that may want to ensure the code continues to function as desired. I would put the pulp_ansible plugin in the non-productized catagory. I ask that QE not devote staffing to writing tests for the pulp_ansible plugin now. I recognize that if that ask changes, it would be nice if they were starting from somewhere above zero test cases and recognize that other stakeholders may have a selfish need to prioritize & possibly do that work. If that non-productized --> productized status changes, happy to revisit policy/planning at that time.
To answer some concern about unit vs. functional testing - I do understand these to be different and distinct. I did not intend to lump these together other in definition or value - just that it would be nice for a contribution to include code+unit test+functional test together. The value I see here is that we still have an option of them being done separately but allow them to be contributed together. Now we have to submit them in 2 different pull requests. If they are merged in 1 pull request we eliminate a scenario where code is in the main branch that fails a pulp-smash test. I could see future policies where bug fixes for pulp-smash issues that are known to fail could be required to re-enable the test - all without QE effort (although I would not have an issue if QE wants to review those changes).
As a proof of concept I have created a PR[0] against pulp_ansible that has some basic pulp-smash tests. The PR also configured Travis to run these tests.
@omaciel and I started evaluating this proposal and its major changes today.
A week from now, we will provide more details on how QE will address this proposal and its impacts.
@kersommoura and @omaciel - Thank you for evaluation. Happy to continue the conversation.
I'm very pleased that @Ichimonji10 articulated a lot of information and detail that is helpful to have written down here. I would like to find a way to encourage community contributions of plugin testing especially for those that I don't see us allocating staff for right now.
@kersommoura I think this one can be closed what do you think?
Yeah.
@kersommoura, @rochacbruno, et. al — just want to thank you for your work on this!