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

Adapt plugin for Data Table API 2.0.x #642

Merged
merged 20 commits into from
Apr 26, 2024

Conversation

mPokornyETM
Copy link
Contributor

@mPokornyETM mPokornyETM commented Apr 5, 2024

fixes #634

Do not mixed Jenkins and data-table css classes.

closes #647

Testing done

Tested also with dark mode and it looks fine.

Proposed upgrade guidelines

N/A

Localizations

N/A

Submitter checklist

  • The Jira / Github issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • The changelog generator for plugins uses the pull request title as the changelog entry.
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during the upgrade.

@mPokornyETM mPokornyETM requested a review from a team as a code owner April 5, 2024 18:43
@mPokornyETM mPokornyETM added bug ui Features that may impact UI, pages made by the plugin or external UIs (BO, legacy, etc.) labels Apr 5, 2024
@basil
Copy link
Member

basil commented Apr 5, 2024

Hey @mPokornyETM, thanks so much for getting this plugin working with the new version of Data Tables. Unfortunately, those two tests are still failing when running in Plugin Compatibility Tester (PCT). I can reproduce the same failures in this repository with

diff --git a/pom.xml b/pom.xml
index b9cd6a3..e77cae1 100644
--- a/pom.xml
+++ b/pom.xml
@@ -70,8 +70,8 @@
     <dependencies>
       <dependency>
	 <groupId>io.jenkins.tools.bom</groupId>
-	<artifactId>bom-2.387.x</artifactId>
-	<version>2543.vfb_1a_5fb_9496d</version>
+	<artifactId>bom-2.440.x</artifactId>
+	<version>2950.va_633b_f42f759</version>
	 <type>pom</type>
	 <scope>import</scope>
       </dependency>

and running:

  • mvn clean verify -Dtest=org.jenkins.plugins.lockableresources.LockableResourceApiTest#reserveUnreserveApi
  • mvn clean verify -Dtest=org.jenkins.plugins.lockableresources.LockStepTest#unlockButtonWithWaitingRuns

LockableResourceApiTest#reserveUnreserveApi trips a test assertion:

java.lang.AssertionError: 

Expected: is <true>
     but: was <false>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at org.jenkins.plugins.lockableresources.LockableResourceApiTest.reserveUnreserveApi(LockableResourceApiTest.java:41)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:656)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.lang.Thread.run(Thread.java:1583)

org.jenkins.plugins.lockableresources.LockStepTest#unlockButtonWithWaitingRuns simply hangs.

These problems persisted with the latest release of HtmlUnit (4.0.0).

Shortly before these problems occur, Rhino prints JavaScript problems with bootstrap:

Exception class=[org.htmlunit.corejs.javascript.EvaluatorException]
org.htmlunit.ScriptException: missing ( before function parameters. (http://localhost:35677/jenkins/static/f1c98be7/plugin/bootstrap5-api/js/bootstrap.min.js#6)
	at org.htmlunit.javascript.JavaScriptEngine$HtmlUnitContextAction.run(JavaScriptEngine.java:963)
	at org.htmlunit.corejs.javascript.Context.call(Context.java:581)
	at org.htmlunit.corejs.javascript.ContextFactory.call(ContextFactory.java:481)
	at org.htmlunit.javascript.HtmlUnitContextFactory.callSecured(HtmlUnitContextFactory.java:313)
	at org.htmlunit.javascript.JavaScriptEngine.compile(JavaScriptEngine.java:735)
	at org.htmlunit.javascript.JavaScriptEngine.compile(JavaScriptEngine.java:110)
	at org.htmlunit.html.HtmlPage.loadJavaScriptFromUrl(HtmlPage.java:1124)
	at org.htmlunit.html.HtmlPage.loadExternalJavaScriptFile(HtmlPage.java:1015)
	at org.htmlunit.html.ScriptElementSupport.executeScriptIfNeeded(ScriptElementSupport.java:187)
	at org.htmlunit.html.ScriptElementSupport$1.execute(ScriptElementSupport.java:111)
	at org.htmlunit.html.ScriptElementSupport.onAllChildrenAddedToPage(ScriptElementSupport.java:134)
	at org.htmlunit.html.HtmlScript.onAllChildrenAddedToPage(HtmlScript.java:192)
	at org.htmlunit.html.parser.neko.HtmlUnitNekoDOMBuilder.endElement(HtmlUnitNekoDOMBuilder.java:508)
	at org.htmlunit.cyberneko.xerces.parsers.AbstractSAXParser.endElement(AbstractSAXParser.java:303)
	at org.htmlunit.html.parser.neko.HtmlUnitNekoDOMBuilder.endElement(HtmlUnitNekoDOMBuilder.java:454)
	at org.htmlunit.cyberneko.HTMLTagBalancer.callEndElement(HTMLTagBalancer.java:1186)
	at org.htmlunit.cyberneko.HTMLTagBalancer.endElement(HTMLTagBalancer.java:1130)
	at org.htmlunit.cyberneko.filters.DefaultFilter.endElement(DefaultFilter.java:168)
	at org.htmlunit.cyberneko.filters.NamespaceBinder.endElement(NamespaceBinder.java:266)
	at org.htmlunit.cyberneko.HTMLScanner$ContentScanner.scanEndElement(HTMLScanner.java:3362)
	at org.htmlunit.cyberneko.HTMLScanner$ContentScanner.scan(HTMLScanner.java:2055)
	at org.htmlunit.cyberneko.HTMLScanner.scanDocument(HTMLScanner.java:847)
	at org.htmlunit.cyberneko.HTMLConfiguration.parse(HTMLConfiguration.java:336)
	at org.htmlunit.cyberneko.HTMLConfiguration.parse(HTMLConfiguration.java:294)
	at org.htmlunit.cyberneko.xerces.parsers.XMLParser.parse(XMLParser.java:70)
	at org.htmlunit.html.parser.neko.HtmlUnitNekoDOMBuilder.parse(HtmlUnitNekoDOMBuilder.java:750)
	at org.htmlunit.html.parser.neko.HtmlUnitNekoHtmlParser.parse(HtmlUnitNekoHtmlParser.java:199)
	at org.htmlunit.DefaultPageCreator.createHtmlPage(DefaultPageCreator.java:307)
	at org.htmlunit.DefaultPageCreator.createPage(DefaultPageCreator.java:226)
	at org.jvnet.hudson.test.HudsonPageCreator.createPage(HudsonPageCreator.java:53)
	at org.htmlunit.WebClient.loadWebResponseInto(WebClient.java:638)
	at org.htmlunit.WebClient.loadWebResponseInto(WebClient.java:541)
	at org.htmlunit.WebClient.getPage(WebClient.java:459)
	at org.htmlunit.WebClient.getPage(WebClient.java:366)
	at org.htmlunit.WebClient.getPage(WebClient.java:504)
	at org.htmlunit.WebClient.getPage(WebClient.java:486)
	at org.jvnet.hudson.test.JenkinsRule$WebClient.goTo(JenkinsRule.java:2722)
	at org.jvnet.hudson.test.JenkinsRule$WebClient.goTo(JenkinsRule.java:2702)
	at org.jenkins.plugins.lockableresources.TestHelpers.clickButton(TestHelpers.java:78)
	at org.jenkins.plugins.lockableresources.LockStepTest.unlockButtonWithWaitingRuns(LockStepTest.java:764)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:656)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.lang.Thread.run(Thread.java:1583)

These problems persist after disabling JavaScript:

diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java
index 4972e7d..13edf41 100644
--- a/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java
+++ b/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java
@@ -746,6 +746,7 @@ public class LockStepTest extends LockStepTestBase {
         p.setDefinition(new CpsFlowDefinition("lock('resource1') { semaphore('wait-inside') }", true));
 
         JenkinsRule.WebClient wc = j.createWebClient();
+        wc.getOptions().setThrowExceptionOnScriptError(false);
 
         WorkflowRun prevBuild = null;
         for (int i = 0; i < 3; i++) {
diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceApiTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceApiTest.java
index 3a7a126..4a70743 100644
--- a/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceApiTest.java
+++ b/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceApiTest.java
@@ -36,6 +36,7 @@ public class LockableResourceApiTest {
         j.jenkins.setAuthorizationStrategy(new FullControlOnceLoggedInAuthorizationStrategy());
 
         JenkinsRule.WebClient wc = j.createWebClient();
+        wc.getOptions().setThrowExceptionOnScriptError(false);
         wc.login("user");
         TestHelpers.clickButton(wc, "reserve");
         assertThat(LockableResourcesManager.get().fromName("a1").isReserved(), is(true));

This makes no difference.

Per the HtmlUnit maintainer in HtmlUnit/htmlunit#498 (comment):

This will not work at the moment because some deficits of the current js support. […] some js constructs used in the page code that are not supported at the moment. I have some plans to improve the support in the future.

This message was written on August 15, 2022.

I think you have a few options:

  • Remove use of fancy JavaScript features and continue to test with HtmlUnit.
  • Replace HtmlUnit tests with Selenium-based functional tests in the ATH repository (the common pattern for Selenium tests in the Jenkins project) or in this repository (similar to how Dr Hafner's plugins feature Selenium tests in specific plugin repositories)
  • Disable these HtmlUnit tests temporarily and hope that HtmlUnit eventually supports this fancy JavaScript in the future.
  • Abandon automated frontend testing permanently and rely on manual testing.

None of these options seem particularly great, but I believe we should choose a path forward one way or another.

@uhafner
Copy link
Member

uhafner commented Apr 10, 2024

Can you add the explicit version https://github.com/jenkinsci/data-tables-api-plugin/releases/tag/v2.0.3-1 here:

</dependency>

Then we see if there are test failures.

@mPokornyETM
Copy link
Contributor Author

Can you add the explicit version https://github.com/jenkinsci/data-tables-api-plugin/releases/tag/v2.0.3-1 here:

</dependency>

Then we see if there are test failures.

@uhafner it provides some dependency issues see also https://github.com/jenkinsci/lockable-resources-plugin/pull/642/checks?check_run_id=24088006393

@uhafner
Copy link
Member

uhafner commented Apr 22, 2024

I think you are using a too old BOM that does not match your Jenkins version:

  <properties>
    <changelist>999999-SNAPSHOT</changelist>
    <jenkins.version>2.440.1</jenkins.version>
  </properties>

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>io.jenkins.tools.bom</groupId>
        <artifactId>bom-2.387.x</artifactId>
        <version>2543.vfb_1a_5fb_9496d</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>

You should use: <jenkins.version>2.426.3</jenkins.version> or <jenkins.version>2.440.3</jenkins.version> and a matching BOM, like <artifactId>bom-2.426.x</artifactId> or <artifactId>bom-2.440.x</artifactId>

@mPokornyETM
Copy link
Contributor Author

  • Remove use of fancy JavaScript features and continue to test with HtmlUnit.
  • Replace HtmlUnit tests with Selenium-based functional tests in the ATH repository (the common pattern for Selenium tests in the Jenkins project) or in this repository (similar to how Dr Hafner's plugins feature Selenium tests in specific plugin repositories)
  • Disable these HtmlUnit tests temporarily and hope that HtmlUnit eventually supports this fancy JavaScript in the future.
  • Abandon automated frontend testing permanently and rely on manual testing.

thx for your time @basil . I mont realy happy :-( No one of the option is fast and good.

about java script. This failures are comming from other plugin, therefore I can not remoive it
about selenium. Never did it, I have no experience with the tool, Some example or tutorial for dummies will be great
disable HtmlUnit tests. Disable is not good, I dont want to lose code coverage on changes. I am still wating for very long time for the update in the HtmlUnit frame.
Manual testing is no more option for me. Not on every change like bom updates

@mPokornyETM mPokornyETM reopened this Apr 26, 2024
@basil
Copy link
Member

basil commented Apr 26, 2024

about java script. This failures are comming from other plugin, therefore I can not remoive it

Agreed; this option is tantamount to dropping your dependency on the other plugins that use advanced JavaScript.

about selenium. Never did it, I have no experience with the tool, Some example or tutorial for dummies will be great

@uhafner is probably the best person to talk about how to run this from a Jenkins plugin repository since I don't think anyone else does this. For writing tests within the ATH repository there is plenty of documentation at https://github.com/jenkinsci/acceptance-test-harness.

disable HtmlUnit tests. Disable is not good, I dont want to lose code coverage on changes. I am still wating for very long time for the update in the HtmlUnit frame.

Unfortunately, HtmlUnit is increasingly unsuitable for modern JavaScript: HtmlUnit/htmlunit#755 jenkinsci/jenkins-test-harness#569

This would be my preferred short-term approach, and is consistent with the status quo, although it is not great from a testing perspective.

Manual testing is no more option for me. Not on every change like bom updates

For what it's worth, BOM updates of things rarely break frontends. The recent incompatible release of Data Tables is an exception to what is otherwise a fairly consistent track record of compatibility.

@mPokornyETM mPokornyETM enabled auto-merge (squash) April 26, 2024 19:42
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

I have confirmed that this allows the exclusions from jenkinsci/bom#3066 to be removed by running

PLUGINS=lockable-resources TEST=org.jenkins.plugins.lockableresources.LockableResourceApiTest#reserveUnreserveApi,org.jenkins.plugins.lockableresources.LockStepTest#unlockButtonWithWaitingRuns bash local-test.sh

@@ -81,6 +81,7 @@
<dependency>
<groupId>io.jenkins.plugins</groupId>
<artifactId>data-tables-api</artifactId>
<version>2.0.5-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.

Can be omitted since this plugin is in BOM.

@mPokornyETM mPokornyETM merged commit f48745d into jenkinsci:master Apr 26, 2024
16 checks passed
@mPokornyETM mPokornyETM deleted the feature/data-table-2.0.x branch April 29, 2024 06:55
basil added a commit that referenced this pull request Apr 30, 2024
basil added a commit to basil/lockable-resources-plugin that referenced this pull request Apr 30, 2024
basil added a commit that referenced this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ui Features that may impact UI, pages made by the plugin or external UIs (BO, legacy, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adapt plugin for Data Table API 2.0.x
3 participants