-
Notifications
You must be signed in to change notification settings - Fork 229
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
Additional test coverage #1053
Comments
I would recommend to move those tests to the corresponding plugins and not to the ATH. At least that was the consensus that we had quite a couple of years ago. It would only make sense to add more tests here that actually test core functionality that is not tested here yet. Otherwise we are bloating up the ATH again. @olivergondza introduced a successful concept for UI tests in plugins that should work here as well. I am also using this concept in all of my plugins very successfully. |
not to my knowledge or recollection. In fact I would say the ATH tests for the warnings-ng are only in the warnings-ng repo is an anti-pattern as they are not picking up regressions caused by changes in Jenkins in a reasonably timely fashion (they are not run as part of the PCT). Having some UI tests in the plugin code is reasonable a it allows the testing for regressions in plugin code when developing the plugin - but there also needs to be some coverage in the ATH. Additionally and this is a not the current state - the ATH tests here should really be exercising human flow now individual plugins - however that ship has sailed and is a larger effort that can be done in the future.
core to what Jenkins or plugins? if Jenkins then I disagree strongly. for plugins, Yes we should not be duplicating existing tests as I wrote "the tests do not significantly overlap." |
It started with #556
Yes, I think a single test per important plugin would be helpful.
Why? There are so many areas in Jenkins core that are not UI tested. And for instance, the new Ant tests add new integration tests for Ant here in the ATH, these should be part of the Ant plugin, not ATH. (They even can be implemented as integration tests without UI layer). |
FWIW I think the tests should be in the plugins and then something similar to PCT should run the tests / a selection of them from here. That way tests in the plugins pickup on regressions and aren't run in isolation and tests here pickup on core issues. |
Yes, that would be the best approach. As far as I understand the PCT, all tests of each participating plugin are executed (even the unit and architecture tests), that makes not really sense. Maybe we should allow plugins to mark tests which get executed in PCT. These tests should include integration tests and UI tests. |
OT: I have been using containerized web driver since long before this change :-/
there are also so many parts of plugin UIs that are not tested. I am not saying we should not add more testing of Jenkins core - but that any added testing should not be exclusively for Jenkins core.
whilst exercising the UI done in a unit test using HTML Unit it is in my experience more brittle and costly to maintain. We could use what you have in
In summary I do not believe we are in a position today to have UI tests in plugins code to pick up on regressions. Until we are I believe that putting UI tests into plugin code this the wrong path. I am not objecting to anyone wanting to do the work to make this happen, but until we have it in place I think blocking any submissions using "should be in plugins repo" is wrong. |
Did you already talk with the authors of the plugins that you are adding new tests for? Are they willing to take over the maintenance of these new tests here in the ATH? One of the reasons that Oliver and I started the transition of tests to individual plugins was the problem that breaking tests were never noticed by the plugin authors here and so the ATH degenerated. Currently we are are green 🎉 , but this requires that someone actively maintains these tests. |
There was always this dilemma coined by @uhafner and @jtnord: We want those tests maintained by component (plugin/core) owners, but they would not do it when tests are hosted in this repo. If we move all the tests to those repos, then ATH becomes toothless in verifying LTS, plugin sets and likely even the cloudbees offering mentioned. We also want ATH to be rich in providing solid grantees, but we want it executed fast and have low maintenance cost at the same time. So we put together guidelines of what tests should go to ATH, and what should go to the plugin repo: https://github.com/jenkinsci/acceptance-test-harness/blob/master/docs/CONTRIBUTING.md#test-contribution. And as Ulli have said, we have a way to have ATH based tests in plugin core (for plugins that are not popular enough, or that are using ATH extensively enough so - if hosted inside ATH repo - it would cause ATH execution to slow down (beyond what is reasonable from community perspective)). I lost touch with ATH long time ago, though I still see this distinction to be a valid one. Maybe the list of use-cases defined thereof needs updating, and as a result the guidelines too - I cannot tell. @jtnord, albeit no longer an ATH contributor - I only use ATH as lib in plugin tests, I support the donation of the tests proposed by you (Thanks!). Although I advise this to be done only where this is in accordance with the guidelines. |
I am sorry, but this already is in place for years. IIRC this has happened before you stepped in, and it permitted us to move away plugin suites with to many tests and too few installs that ware simply not worth keeping up-to-date with plugin codebase by ATH maintainers (putting aside that all regressions were found post-release anyway). Some of the examples - except for those mentioned by @uhafner:
Please, don't :) |
Randomly aside there can always be a 3rd way - the same tests can be in here and in a plugin - so they are excursive by the plugin (so they are maintained) and they are run on plugin development - we just need to publish the test-jar and then get the plugin to depend on the test jar and run tests from here - with some surefire config to only run tests that it wants via include patterns or via adding categories to the tests where a category is a plugin involved. (or any other way). Main issue could be that we only strive to maintain compatability with the current LTS and weekly - and plugins may be using something older - as well as only a few older versions of plugins - but maybe that is enough (we don;t actually aggressivly cleanup old LTS support right now - there is some things for
Now I know it is used I won't remove it without a tested replacement. However I have tried numerous times to reproduce / fix #1090 but have been unable to reproduce it - so am unable to debug or fix it. I still think there is some value to be had in removing it and using test containers to start and inject a remote-webdriver-url and this would fix the issue of host-based networking as well - but replacing it is no where near a priority for me :) |
I am using actively dependabot to upgrade the Jenkins version in my plugin UI tests. This does not detect broken Jenkins PRs, but at least it detects broken Jenkins releases one week later. Example: jenkinsci/warnings-ng-plugin#1550
This is not the way it works currently. The Jenkins version for UI tests is different from the Jenkins version for the build. So it will detect binary incompatibilities with newer Jenkins versions. |
What feature do you want to see added?
CloudBees has maintained it's own extended set of tests for some open source plugins.
These internal tests don't benefit the community as much as they could as there is no visibility of them outside of CloudBees.
They are run on every build of our product (many times per day) without any surefire re-runs enabled and so are considered as stable as the ATHs in this repository.
We intend to open source these tests by migrating them to this repository where the tests do not significantly overlap.
This ticket is so you are aware of this upcoming work @timja @NotMyFault
Upstream changes
No response
The text was updated successfully, but these errors were encountered: