Inline form element path #5926

timja · 2021-11-17T21:34:21Z

See jenkinsci/form-element-path-plugin#11 (comment)

This inlines form-element-path so that we can change it as part of UI re-work without having to update a separate test tool and get it released. It will also enable htmlunit testing with more stable selectors which will be nice.

It is enabled by:

default in unit tests
system property jenkins.formelementpath.FormElementPathPageDecorator.enabled=true *setting jenkins.formelementpath.FormElementPathPageDecorator.ENABLED = true` via script console.

ATH is setting system property with jenkinsci/acceptance-test-harness#712, all ATH tests are passing with this.

Plugin is EOLed in jenkinsci/form-element-path-plugin#12

For anyone who don't know what this is it's a plugin that provides an attribute on the DOM which is based on the 'path' to an attribute:

Proposed changelog entries

Add path to form elements giving stable selectors for UI testing.

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

core/src/main/resources/jenkins/formelementpath/FormElementPathPageDecorator/footer.jelly

core/src/main/java/jenkins/formelementpath/FormElementPathPageDecorator.java

jglick

whether it should be enabled by default or with a system property

No strong opinion. Seems like it would be simpler overall to have this enabled unconditionally, so we could inline these additions into regular controls (probably generated server-side) rather than all these tricks with Behavior. If we are concerned about HTML bloat and do not want it on by default, then it could be gated by a system property, defaulting to Main.isUnitTest perhaps.

core/src/main/resources/jenkins/formelementpath/form-element-path.js

jglick · 2021-11-17T22:46:21Z

core/src/main/resources/jenkins/formelementpath/form-element-path.js

+    }
+
+    // expose this globally so that Selenium can call it
+    window.recomputeFormElementPath = applyAll;


Will this blow up somehow if the plugin is also installed?

I plan to bump the baseline of the plugin and empty it out initially so this shouldn’t really matter

it wouldn't blow up anyway just last one to register would win

timja · 2021-11-18T08:28:22Z

rather than all these tricks with Behavior

I've removed all the prototype.js code, we could inline this into hudson-behaviour.js#buildFormTree as well, but would rather keep it separate initially

timja · 2021-11-18T08:55:02Z

core/src/main/resources/jenkins/formelementpath/form-element-path.js

+    applyAll();
+
+    // run this periodically to cope with DOM changes
+    window.setInterval(applyAll, 1000);


this is what I'm most worried about having it on by default, every second recalculating the paths.

I tried removing it but its needed for repeatable elements to have the index added and re-calculated.

Jenkins already has runtime performance issues and periodic queries, I would avoid this as much as possible.

I'd suggest maybe registering this as a layout update callback (

jenkins/war/src/main/webapp/scripts/hudson-behavior.js

Line 2487 in 3e8afc5

var layoutUpdateCallback = {

). Most widgets already use it after adding new stuff (https://github.com/jenkinsci/jenkins/search?q=layoutUpdateCallback). Only repeatable is missing I think.

it's already registered a couple of lines down, I'll take a look at adding it to repeatable to see if it removes the need for this.

It should probably fix it for most plugins. Don't know if it's a problem that rogue plugins with custom widgets do not have this tbh

Jenkins already has runtime performance issues and periodic queries, I would avoid this as much as possible.

As in #5926 (review) you should either

Add the new HTML attributes or whatever unconditionally during Jelly generation, deleting all the JavaScript hacks, or at least as part of buildFormTree so it happens synchronously if and when a form is submitted.

Or disable all of this by default in production code.

Will probably disable by default but also look to add repeatable to layout update callback so the timer bit can be removed.

The timer bit made debugging this code a pain.

Maybe start with the current code copied-and-pasted from the plugin but disabled by default. That at least solves the immediate issue, we can gut the plugin, and simplify ATH to not try to load it.

Then as a follow-up look into whether the actual behavior can be simplified by inlining into lib/form/*.jelly and/or hudson-behavior.js.

timja · 2021-11-19T21:41:44Z

Review comments should be all addressed now.

a PR to adjust ATH:
jenkinsci/acceptance-test-harness#712

timja · 2021-11-22T21:59:28Z

https://github.com/jenkinsci/acceptance-test-harness/pull/712/checks?check_run_id=4291377477 shows that something here isn't working, likely it's the removal of the run every second. I'll see if there's a fix otherwise I'll restore it since this is disabled by default now

timja · 2021-11-23T08:31:09Z

The general case was failing because yui-button reworks the DOM, didn't really want to patch the library so I've just re-ran apply after 1 second.

Other case I noticed was pipeline code mirror, same workaround fixes that.

timja · 2021-11-23T16:48:32Z

One plugin left failing atm: run_groovy – plugins.GroovyPluginTest

Will see if patchable otherwise will restore the run every second hack I guess

timja · 2021-11-24T10:06:08Z

All is passing now in ATH, @jtnord can you take a look please?

jtnord

Thanks Tim, this looks like a great improvement.

Would like a review by @Vlatombe before this merges - as the enabling via system property may cause some issues for some of our testing.

jtnord · 2021-11-24T11:07:23Z

core/src/main/java/jenkins/formelementpath/FormElementPathPageDecorator.java

+public class FormElementPathPageDecorator extends PageDecorator {
+
+    @SuppressFBWarnings("MS_SHOULD_BE_FINAL")
+    private static /*almost final */ boolean ENABLED = Main.isUnitTest ||


I wonder if we should still be able to flip this switch via the installation of a plugin?
Currently we spin up some Jenkins' instance where we do not configure system properties (or doing so is hard) and the current flow is to install the form-element-path plugin without the use of the paths it provides. (e.g. auto provisioning in kubernetes)

@Vlatombe knows more about this area of the framework that we use internally.

That's fine, we'll be able to provide the system property.

Could also make it public as well

as Vincent is ok with being able to provide it, we can keep it as it is for now (easier to open up, than restrict accesses later)

I think we need to turn this off by default, even in tests, at least until it can be rewritten to not rely on DOM listeners: jenkinsci/bom#746 (comment)

Ah that’s a shames :(

jtnord · 2021-11-24T11:08:26Z

core/src/main/java/jenkins/formelementpath/FormElementPathPageDecorator.java

+
+    @SuppressFBWarnings("MS_SHOULD_BE_FINAL")
+    private static /*almost final */ boolean ENABLED = Main.isUnitTest ||
+            SystemProperties.getBoolean(FormElementPathPageDecorator.class.getName() + ".enabled");


possible future enhancement would be to also set this for RealJenkinsRule

That should set isUnitTest IIRC.

jtnord · 2021-11-24T11:14:06Z

core/src/main/resources/jenkins/formelementpath/form-element-path.js

+    }
+
+    /* JavaScript sometimes re-arranges the DOM and doesn't call layout callback
+     * known cases: YUI buttons, CodeMirror.


Ahhhh!!!! I wonder if this may well explain some issues we have with codemirror sections not being found in the ATH!

timja · 2021-11-24T15:12:57Z

@jglick your comments should be addressed now if you want to take another look

timja · 2021-11-24T18:51:55Z

or @Vlatombe you able to approve?

timja · 2021-11-24T18:55:57Z

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

timja · 2021-11-24T22:42:09Z

PR to form-element-path to kill it off: jenkinsci/form-element-path-plugin#12

timja · 2021-11-26T08:08:05Z

Example using form element path to make an HtmlUnit test more stable / easier to maintain:
9281ae8

This test broke every time markup was touched a little bit

Inline form element path

45bb4c7

timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted developer Changes which impact plugin developers labels Nov 17, 2021

timja requested review from jglick, jtnord, fqueiruga and daniel-beck November 17, 2021 21:34

timja commented Nov 17, 2021

View reviewed changes

core/src/main/resources/jenkins/formelementpath/FormElementPathPageDecorator/footer.jelly Outdated Show resolved Hide resolved

timja commented Nov 17, 2021

View reviewed changes

core/src/main/java/jenkins/formelementpath/FormElementPathPageDecorator.java Show resolved Hide resolved

Apply suggestions from code review

2546e62

jglick reviewed Nov 17, 2021

View reviewed changes

Clean up code

a5cb4cb

Add docs

dbc3407

timja commented Nov 18, 2021

View reviewed changes

timja marked this pull request as draft November 18, 2021 14:19

timja added 3 commits November 19, 2021 08:55

Check element has a classList

d9e1c10

Don't call update callback all the time

0c6aac1

Disable by default

a69fec3

timja marked this pull request as ready for review November 19, 2021 21:41

timja requested a review from jglick November 19, 2021 21:41

Merge branch 'master' into inline-form-element-path

035305b

Add workaround for yui-button and pipeline text box

f6069df

Hardening

0c5b817

timja mentioned this pull request Nov 24, 2021

Modernise form components #5923

Merged

10 tasks

timja requested review from Vlatombe and a team November 24, 2021 10:06

jtnord approved these changes Nov 24, 2021

View reviewed changes

Vlatombe approved these changes Nov 24, 2021

View reviewed changes

timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 24, 2021

timja mentioned this pull request Nov 24, 2021

Kill plugin jenkinsci/form-element-path-plugin#12

Merged

timja merged commit b55eb6b into jenkinsci:master Nov 25, 2021

timja deleted the inline-form-element-path branch November 25, 2021 08:45

This was referenced Dec 3, 2021

Add Email Extension to the managed set jenkinsci/bom#551

Merged

[updatecli] Update weekly version jenkinsci/bom#746

Merged

jtnord mentioned this pull request May 27, 2022

Make it work again jenkinsci/acceptance-test-harness#776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline form element path #5926

Inline form element path #5926

timja commented Nov 17, 2021 •

edited by MarkEWaite

Loading

jglick left a comment

jglick Nov 17, 2021

timja Nov 18, 2021

timja Nov 19, 2021

timja commented Nov 18, 2021

timja Nov 18, 2021

fqueiruga Nov 18, 2021

timja Nov 18, 2021

fqueiruga Nov 18, 2021

jglick Nov 18, 2021 •

edited

Loading

timja Nov 18, 2021 •

edited

Loading

jglick Nov 18, 2021

timja commented Nov 19, 2021

timja commented Nov 22, 2021

timja commented Nov 23, 2021

timja commented Nov 23, 2021

timja commented Nov 24, 2021

jtnord left a comment

jtnord Nov 24, 2021

Vlatombe Nov 24, 2021

timja Nov 24, 2021

jtnord Nov 24, 2021

jglick Dec 3, 2021

timja Dec 3, 2021

jglick Dec 3, 2021

jtnord Nov 24, 2021

jglick Nov 29, 2021

jtnord Nov 24, 2021

timja commented Nov 24, 2021

timja commented Nov 24, 2021

timja commented Nov 24, 2021

timja commented Nov 24, 2021

timja commented Nov 26, 2021 •

edited

Loading

Inline form element path #5926

Inline form element path #5926

Conversation

timja commented Nov 17, 2021 • edited by MarkEWaite Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

timja Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Nov 19, 2021

timja commented Nov 22, 2021

timja commented Nov 23, 2021

timja commented Nov 23, 2021

timja commented Nov 24, 2021

jtnord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Nov 24, 2021

timja commented Nov 24, 2021

timja commented Nov 24, 2021

timja commented Nov 24, 2021

timja commented Nov 26, 2021 • edited Loading

timja commented Nov 17, 2021 •

edited by MarkEWaite

Loading

jglick Nov 18, 2021 •

edited

Loading

timja Nov 18, 2021 •

edited

Loading

timja commented Nov 26, 2021 •

edited

Loading