-
Notifications
You must be signed in to change notification settings - Fork 338
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
Feature/UI tests #342
base: master
Are you sure you want to change the base?
Feature/UI tests #342
Conversation
# Conflicts: # pom.xml
What is the reasoning to do this here? |
Note that this will complicate infrastructure somewhat. jenkinsci/plugin-compat-tester#249 |
I think UI tests belong to a plugin like unit and integration tests (the idea is from @olivergondza, see jenkinsci/acceptance-test-harness#556 for details). After contributing to ATH for several years now I made the observation that nobody is actually looking at UI tests that are located somewhere else. So it makes sense to have them in the plugin. Then we see if a new feature breaks an existing feature (on the UI side) or if an upgrade to a more recent core version breaks the UI. Additionally, JUnit plugin uses integration tests with HTMLUnit which does not support modern JS frameworks (see #272, #286) . So it makes sense to remove those tests in favor of real UI tests.
Why should that be a problem? If the tests will break then something in the UI is wrong. That means we should fix it. |
I haven't looked in detail at those changes but I would assume that we would have two alternatives: a single module plugin and a multi-module plugin. So the Would it be possible to just use the plugin module in those tests and ignore the other ones? |
As Ulli said the critical reason for this is modern JS frameworks don't work with HTMLUnit 😢 |
Yes. But this means that PCT (as run from |
Probably belongs in ATH? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments in #355 (review).
GitHub is unable to render this PR in my browser.
Main review comments
- Use the correct AssertJ assertions
- Don't duplicate login in page objects (2 constructors)
It would be also helpful if you can include one additional SmokeTest
that exercises the most important UI elements in one row so that this test can be used during PR builds. (The other UI tests can be then used only as additional tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to finish my review :(
@@ -190,15 +189,5 @@ public String getDisplayName() { | |||
return Collections.unmodifiableSet(context); | |||
} | |||
|
|||
public FormValidation doCheckHealthScaleFactor(@QueryParameter double value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed if the change is only adding UI tets?
Test\ Description=Testbeschreibung | ||
Test\ Duration=Testdauer | ||
Test\ Result=Testergebnis | ||
# The MIT License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something seems odd here...
|
||
Error\ Details=Fehlerdetails | ||
Stack\ Trace=Stacktrace | ||
# The MIT License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again?
Fail=Fehlgeschlagen | ||
Skip=Ausgelassen | ||
Total=Summe | ||
# The MIT License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
@NikolasFunction @Tjuri Are you still planning to update this PR? The longer you are waiting the more merge conflicts will occur... |
Added ui tests for the junit plugin.
https://issues.jenkins.io/browse/JENKINS-67275