[JENKINS-65585] Fix requestSubmit() usage #5479
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 tasks
|
Unfortunately, this changes don't fix the failed tests in Test method:
So, it seems that with this patch, the submit button value gets submitted with the input form (the pipeline gets "approved by Alice"), which is a step in the right direction. But then there is this 404 answer, from what I assume is an attempt at submitting the form again :-/ |
|
Added 0eb9318, which seems to avoid the double-POST issue in |
|
It seems that this problem is reproducible within Jenkins core in diff --git a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java
index d8a4555832..4551f8bebb 100644
--- a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java
+++ b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java
@@ -112,14 +112,14 @@ public class RekeySecretAdminMonitorTest {
// one should see the warning. try scheduling it
assertFalse(monitor.isScanOnBoot());
HtmlForm form = getRekeyForm(wc);
- submit(wc, form, "schedule");
+ j.submit(form, "schedule");
assertTrue(monitor.isScanOnBoot());
form = getRekeyForm(wc);
assertTrue(getButton(form, 1).isDisabled());
// run it now
assertFalse(monitor.getLogFile().exists());
- submit(wc, form, "background");
+ j.submit(form, "background");
assertTrue(monitor.getLogFile().exists());
// should be no warning/error now
@@ -134,7 +134,7 @@ public class RekeySecretAdminMonitorTest {
// dismiss and the message will be gone
assertTrue(monitor.isEnabled());
form = getRekeyForm(wc);
- submit(wc, form, "dismiss");
+ j.submit(form, "dismiss");
assertFalse(monitor.isEnabled());
assertThrows(ElementNotFoundException.class, () -> getRekeyForm(wc));
}
@@ -143,17 +143,6 @@ public class RekeySecretAdminMonitorTest {
return wc.goTo("manage").getFormByName("rekey");
}
- private void submit(JenkinsRule.WebClient wc, HtmlForm form, String name) throws IOException {
- WebRequest request = form.getWebRequest(null);
- /*
- * TODO There is a long-undiagnosed issue with the test harness not being compatible with
- * message.groovy's f.submit. Work around this by matching the behavior of a real browser
- * (adding the desired button to the request as a parameter).
- */
- request.getRequestParameters().add(new NameValuePair(name, ""));
- wc.getPage(request);
- }
-
private HtmlButton getButton(HtmlForm form, int index) {
// due to the removal of method HtmlElement.getHtmlElementsByTagName
Stream<HtmlButton> buttonStream = form.getElementsByTagName("button").stream()With that diff and without the changes in this PR, With that diff and with the changes in this PR, |
|
@res0nance reported in #5470 (comment) that the related PCT issues picked up in the bom new LTS baseline PR are fixed by this change |
|
I've added 8edaa20 earlier to limit potential side-effects of 0eb9318: as a precautionary measure, only do the I think this PR is ready for review. At least it seems to fix something (thanks @res0nance for running the PCT), so I will remove the WIP status. Regarding the And also regarding this "avoiding double-submit in HtmlUnit" subject, I think there could be an alternative way (without In the case of Jenkins' forms, the POSTed body actually changes when the form is double-submitted. Example of what I was getting after b7d8c12 only (tcpdump during the In the second POST body, the
Hope it makes sense. All that to say that if this JS magic was fixed to include the submit button value in the |
thomasgl-orange
changed the title
Merged
6 tasks
timja
approved these changes
May 18, 2021
|
Do we want to ship this in weekly? |
If you do, maybe also include @basil's proposed change to |
can be added in a followup, weekly ships really soon |
|
On-holding for:
|
daniel-beck
added
bug
|
Un-on-holding, I get the same minified version of the JS when regenerating it locally from the debug source. (It's still wrong since we use the debug as reference, which has additional log statements, but that's for another time. It's not a regression in this PR, those statements have been in there since 2014.) |
|
@thomasgl-orange I applied this change to my lts-backport-2.289 branch in commit 175a8407 . I see in your comment that a further change was needed. I've applied that change in MarkEWaite@7f1e804 I've confirmed that it saves from Firefox 88 and from Chrome just as you reported in #5470 (comment) . I spent an hour clicking various other locations in the Jenkins UI with special focus on configuring jobs. I found no issues with that combination of changes. |
|
Don’t we need to add the no validate change in this PR? |
timja
approved these changes
May 19, 2021
Thanks @MarkEWaite for your tests.
@timja: Yes, this last change was a bit too fresh to add in the PR yesterday, but since there is nothing obviously wrong with it after more testing, I've now included it. Note: Can I Use data for |
MarkEWaite
approved these changes
May 19, 2021
There was a problem hiding this comment.
I am not a Javascript programmer, but my interactive testing with Firefox and Chrome found no issue with this. Without this change, I have a matrix job configuration that cannot be saved on either Firefox or Chrome. With this change, the job can be saved.
Thanks very much @thomasgl-orange !
timja
changed the title
|
@thomasgl-orange any chance you can add a proposed changelog entry? |
|
@timja: sure, added a proposed entry at the bottom of the initial PR comment |
res0nance
approved these changes
May 23, 2021
timja
added
the
ready-for-merge
|
This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
timja
pushed a commit
that referenced
this pull request
Jun 14, 2021
Co-authored-by: Basil Crow <[email protected]> (cherry picked from commit 42c5633)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See JENKINS-65585 and #5405.
This is where I'm at, trying to untangle this 2.289 regression with HtmlUnit tests on some submit buttons.
What I've understood so far:
<button type="submit" />or<input type="submit" />(otherwise we get a JS exception)submitForm()function (not sure my method is always correct, nor that it is the simplest one)type="button", because:lib/form/submit.jellyproduces an<input type="submit"/>(so far so good)scripts/hudson-behavior.jsYUIfy it by callingYAHOO.widget.Buttonyui/button-*.js) produces a<button type="button"/>, ignoring the original typeSo, that's two (scary) changes in
yui/button-*.js. It seems to fix the test regression onTrustHostKeyActionTestinssh-slaves, but is not much tested otherwise. Hence this PR, to get a first CI run (I don't expect it to be blue).FTR, the
button-min.jshas been regenerated withyuicompressor(plus copy/paste of the copyright header):It gives two unmodified lines, and then a bunch of obfuscated minified code which should obviously be regenerated by a maintainer if this ever gets merged.
Proposed changelog entries