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

Feature/UI tests #342

Draft
wants to merge 97 commits into
base: master
Choose a base branch
from

Conversation

NikolasFunction
Copy link

@NikolasFunction NikolasFunction commented Jan 20, 2022

Added ui tests for the junit plugin.
https://issues.jenkins.io/browse/JENKINS-67275

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@uhafner uhafner added the test A PR that adds to testing - used by Release Drafter label Feb 16, 2022
@uhafner uhafner self-requested a review February 16, 2022 18:29
@jtnord
Copy link
Member

jtnord commented Feb 17, 2022

What is the reasoning to do this here?
UI testing will be brittle with changes in Jenkins core so the Jenkins tools BOM will likely be failing regularly for weeklies, and or versions held back from the BOM for some lines.

@jglick
Copy link
Member

jglick commented Feb 17, 2022

Note that this will complicate infrastructure somewhat. jenkinsci/plugin-compat-tester#249

@uhafner
Copy link
Member

uhafner commented Feb 18, 2022

What is the reasoning to do this here?

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.

UI testing will be brittle with changes in Jenkins core so the Jenkins tools BOM will likely be failing regularly for weeklies, and or versions held back from the BOM for some lines.

Why should that be a problem? If the tests will break then something in the UI is wrong. That means we should fix it.

@uhafner
Copy link
Member

uhafner commented Feb 18, 2022

Note that this will complicate infrastructure somewhat. jenkinsci/plugin-compat-tester#249

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 ifWarnings blocks should be actually ifMultiModule blocks.

Would it be possible to just use the plugin module in those tests and ignore the other ones?

@timja
Copy link
Member

timja commented Feb 18, 2022

As Ulli said the critical reason for this is modern JS frameworks don't work with HTMLUnit 😢

@jglick
Copy link
Member

jglick commented Feb 18, 2022

Would it be possible to just use the plugin module in those tests and ignore the other ones?

Yes. But this means that PCT (as run from bom) will not verify that changes to core and plugin deps do not break the UI of the junit plugin. Basically the trick of including ATH-based tests inside a plugin repo is not currently supported by other tooling.

@timja
Copy link
Member

timja commented Feb 18, 2022

Basically the trick of including ATH-based tests inside a plugin repo is not currently supported by other tooling.

Probably belongs in ATH?

Copy link
Member

@uhafner uhafner left a 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).

Copy link
Member

@jtnord jtnord left a 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) {
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

?

@uhafner
Copy link
Member

uhafner commented Apr 23, 2022

@NikolasFunction @Tjuri Are you still planning to update this PR? The longer you are waiting the more merge conflicts will occur...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test A PR that adds to testing - used by Release Drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants