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

[JENKINS-53511] Improve discovery and readability of WebClient #3618

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Sep 11, 2018

See JENKINS-53511.

Proposed changelog entries

  • Developer: Improve discovery and readability of WebClient most popular options.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

This cleans things up nicely.

@oleg-nenashev oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Sep 27, 2018
@oleg-nenashev oleg-nenashev added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 28, 2018
@oleg-nenashev
Copy link
Member

@Wadeck are you still interested in it? There are merge conflicts, and there will be more if it is not merged soon

@Wadeck Wadeck force-pushed the JENKINS-53511_WEBCLIENT_DISCOVERY_IMPROVEMENT branch 2 times, most recently from cc1a05e to d97361b Compare January 29, 2019 09:46
@Wadeck
Copy link
Contributor Author

Wadeck commented Jan 29, 2019

For the information, the upgrade from jenkins-test-harness:2.41.1 to jenkins-test-harness:2.46 had at least 2 broken backward compabitility issues:

  • XmlPage removed the method getContent that was deprecated, so I replace it call by getWebResponse().getContentAsString(), in ViewTest.
  • WebResponse changed the signature of the method getContentAsString(String) by getContentAsString(Charset), so I replaced "UTF-8" by StandardCharsets.UTF_8 in ComputerTest.

@Wadeck Wadeck removed on-hold This pull request depends on another event/release, and it cannot be merged right now unresolved-merge-conflict There is a merge conflict with the target branch. labels Jan 29, 2019
@Wadeck Wadeck removed their assignment Jan 29, 2019
@oleg-nenashev
Copy link
Member

For the information, the upgrade from jenkins-test-harness:2.41.1 to jenkins-test-harness:2.46 had 2 broken backward compabitility issues:

It has broken even more. I tried updating from 2.41 to 2.42 in #3661 , but finally I gave up and released 2.41.1

@Wadeck
Copy link
Contributor Author

Wadeck commented Jan 29, 2019

It has broken even more

Seems I have only discovered the top of the iceberg :p will try to correct them...

Other breakages:

  • WebResponse#getContentCharset() returned a String, now it's a Charset.
  • HtmlElement#getHtmlElementsByTagName(String) was removed
  • JavaScriptEngine#callFunction signature changed

=> "only" 64 tests failures (= 16 as we have 4 different env).

  • Due to the space added in jenkins-test-harness#89 the temp folder is causing troubles (both linux and windows) for the test hudson.model.ItemsTest#overwriteNonexistentTarget. This test is passing with 2.42, but not with 2.46.
    • The problem with the space is that StreamResult#new convert the file to URI and so, convert the space into %20, that is not recognized as a file.
    • Only failing when trying to use the itemGroupMixIn.createTopLevelItem(req, rsp);
    • To solve those problems, I created a special JenkinsRuleWithoutSpace that save/write/reload the system property to be compliant with the legacy behavior.
  • Same for all except (need further investigation)
    • hudson.model.ProjectTest#testGetScmCheckoutRetryCount
      • Change in the way the click is done on an element, the form does not receive the information and so there is a missing boolean in the form submission.
      • Using .click(new Event(), true) has the legacy behavior, so it's the new ignoreVisibility option with a default value that change the legacy behavior. Introduced in 2.42 with the update of HtmlUnit (I bet on jenkins-test-harness#103).
    • jenkins.bugs.Jenkins19124Test#interrelatedFormValidation
      • The setValueAttribute is no longer triggering the JavaScript onChange event, so I needed to trigger it manually (legacy behavior present until 2.42)
    • lib.form.PasswordTest#testCheckMethod
      • Like setValueAttribute but for setText of password fields

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

test-pom/pom.xml Outdated
@@ -54,7 +54,9 @@ THE SOFTWARE.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>2.41.1</version>
<!-- <version>2.41.1</version>-->

Choose a reason for hiding this comment

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

Are these versions commented on purpose?

Copy link
Contributor Author

@Wadeck Wadeck Jan 29, 2019

Choose a reason for hiding this comment

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

I am still correcting the tests, so it's WiP (label added)

Copy link
Contributor Author

@Wadeck Wadeck Jan 29, 2019

Choose a reason for hiding this comment

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

Ho actually I thought I pushed the 2.46 after the failing try with 2.42. Thx for the heads up!

Copy link
Member

Choose a reason for hiding this comment

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

I think this bump should actually be reverted in not required for the scope of that PR. This seems like something that can (and if can, then should) be handled in a dedicated PR for clarity (and more speedy reviews because less code, good for everyone).

Choose a reason for hiding this comment

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

This has been marked as ready for merge but the code is still commented. Shouldn't we just remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- <version>2.41.1</version>-->

@Wadeck Wadeck added the work-in-progress The PR is under active development, not ready to the final review label Jan 29, 2019
@Wadeck
Copy link
Contributor Author

Wadeck commented Jan 29, 2019

Next step, try using system property to enforce the withoutSpace behavior to avoid troubles with the temp directories.

@batmat
Copy link
Member

batmat commented Jan 30, 2019

in 72bafd9

Re-put the 2.46 that is the target

Why is this actually the target?

@Wadeck Wadeck force-pushed the JENKINS-53511_WEBCLIENT_DISCOVERY_IMPROVEMENT branch from 0e643c0 to 297104a Compare January 30, 2019 14:13
@Wadeck
Copy link
Contributor Author

Wadeck commented Jan 30, 2019

As discussed with Baptiste, this PR will only focus on inclusion of the JTH version that provide the initial desired methods, not all the change with the space issue. So putting the JTH to 2.42 only.

Due to this update, I was forced to bump as well the commons-io lib to 2.6. It's fully compatible with 2.4 cf upgrade to 2.5 and upgrade to 2.6.

@Wadeck Wadeck mentioned this pull request Jan 30, 2019
4 tasks
@jglick
Copy link
Member

jglick commented Jan 30, 2019

The problem with the space is that StreamResult#new convert the file to URI and so, convert the space into %20, that is not recognized as a file.

Sounds like a legitimate bug, or what am I missing?

@Wadeck
Copy link
Contributor Author

Wadeck commented Jan 31, 2019

Sounds like a legitimate bug, or what am I missing?

The constructor transforms the File into URI but that's the good behavior at this point. I thought it was the root cause but actually it's the interpretation of this URI that is badly done. That bad interpretation is only present in the Xalan version included in the JTH.

I tested JTH-2.42 with / without space, same behavior. Using hpi:run, (with JTH:2.42) it works with and without space.

The JTH:2.46 has trouble with space but not without. And if I run the hpi:run with JTH:2.46, it works with and without space.

Conclusion, it's only a problem in test and so, not a "production" bug.


Thanks @thomasgl-orange for the so simple PR that solves my annoying issue!
Are you OK if we merge this PR and then you can adjust yours to just rebase it on the master after the merge? As it's that simple (removal of a dep) it should be easy. Otherwise I can integrate your change here, let me know what you prefer!

@thomasgl-orange
Copy link
Contributor

Are you OK if we merge this PR and then you can adjust yours to just rebase it on the master after the merge?

Sure, sounds good, once this one gets merged I will update mine.

@Wadeck Wadeck requested a review from jvz January 31, 2019 09:42
@Wadeck Wadeck requested a review from jeffret-b January 31, 2019 09:43
@Wadeck
Copy link
Contributor Author

Wadeck commented Jan 31, 2019

I revoked the previous approval as I made changes since their creation

Copy link
Member

@fcojfernandez fcojfernandez left a comment

Choose a reason for hiding this comment

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

It still looks good to me!

@Wadeck
Copy link
Contributor Author

Wadeck commented Feb 1, 2019

Seems we are good with this one, I will request a merge later today if nobody complains!

@Wadeck Wadeck added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed work-in-progress The PR is under active development, not ready to the final review labels Feb 1, 2019
Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

two comments to remove on test/pom.xml and it's good.

test-pom/pom.xml Outdated
@@ -54,7 +54,9 @@ THE SOFTWARE.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>2.41.1</version>
<!-- <version>2.41.1</version>-->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- <version>2.41.1</version>-->

test-pom/pom.xml Outdated
<version>2.41.1</version>
<!-- <version>2.41.1</version>-->
<version>2.42</version>
<!-- <version>2.46</version>-->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- <version>2.46</version>-->

Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

I don't understand why Github showed me outdated content in the review..

@Wadeck
Copy link
Contributor Author

Wadeck commented Feb 1, 2019

Seems that Isa got the same issue

@Wadeck Wadeck force-pushed the JENKINS-53511_WEBCLIENT_DISCOVERY_IMPROVEMENT branch from 297104a to 73c6974 Compare February 1, 2019 14:18
@Wadeck
Copy link
Contributor Author

Wadeck commented Feb 1, 2019

Squash of the commits to separate clearly what was required for the JTH bump (due to breaking change in HTMLUnit new version) and what's for the discovery of the new features.

@vilacides
Copy link

All good! 👍

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Thanks @Wadeck ! Will merge once the tests pass.

@oleg-nenashev
Copy link
Member

i doubt this is related, but...

Stacktrace
java.lang.AssertionError
at hudson.slaves.PingThreadTest.failedPingThreadResetsComputerChannel(PingThreadTest.java:90)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:552)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.lang.Thread.run(Thread.java:834)

@batmat
Copy link
Member

batmat commented Feb 3, 2019

I strongly suspect Oleg was planning to close/reopen to retrigger the build.

@batmat batmat reopened this Feb 3, 2019
@oleg-nenashev
Copy link
Member

@batmat Definitely. Sorry for forgetting about the second step :(

@batmat
Copy link
Member

batmat commented Feb 3, 2019

No problem at all Oleg. It's happened to me many times :)

@Wadeck
Copy link
Contributor Author

Wadeck commented Feb 4, 2019

And now it's passing, thx @batmat / @oleg-nenashev

@oleg-nenashev
Copy link
Member

That's why I do poke commits instead of close/reopen :(

@oleg-nenashev oleg-nenashev self-assigned this Feb 7, 2019
@oleg-nenashev
Copy link
Member

Will merge it once the build passes

@oleg-nenashev oleg-nenashev merged commit fab1662 into jenkinsci:master Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants