[JENKINS-74025] Extract inline JavaScript from checkboxContent.jelly

[yaroslavafenkin]

https://issues.jenkins.io/browse/JENKINS-74025

Testing done

Created a freestyle project with 2 Active Choices Parameters. Took sample Groovy scripts from the README. Choice type - Check Boxes.

Before the change
After the change

Submitter checklist

• 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

[kinow]

Thanks for the PRs to update this code pattern on this repo, @yaroslavafenkin !

I was looking at the changes on these PRs, and it occurred to me that if we moved the inline functions to this file as a function, then we could call that in these forEach loop, while also being able to write tests to verify those functions. WDYT?

[yaroslavafenkin]

Sure, I could look into that.

[yaroslavafenkin]

I'm trying to figure out to approach this.

If I just extract the function like this:

Index: src/main/resources/org/biouno/unochoice/common/checkbox-content.js
<+>UTF-8
===================================================================
diff --git a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
--- a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js	(revision 91aee0743ce607987513b2823e23dfdbc6da2135)
+++ b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js	(date 1731325784805)
@@ -2,18 +2,22 @@
     document.querySelectorAll(".checkbox-content-data-holder").forEach((dataHolder) => {
         const { maxCount, randomName } = dataHolder.dataset;
 
-        var height = 0;
-        var refElement = document.getElementById(`ecp_${randomName}_0`);
-        if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
-            for (var i = 0; i < maxCount; i++) {
-                height += refElement.offsetHeight + 3;
-            }
-        }
-        else {
-            height = maxCount * 25.5;
-        }
+        setHeight(maxCount, randomName);
+    });
+});
+
+function setHeight(maxCount, randomName) {
+    var height = 0;
+    var refElement = document.getElementById(`ecp_${randomName}_0`);
+    if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
+        for (var i = 0; i < maxCount; i++) {
+            height += refElement.offsetHeight + 3;
+        }
+    }
+    else {
+        height = maxCount * 25.5;
+    }
 
-        height = Math.floor(height);
-        document.getElementById(`ecp_${randomName}`).style.height = height + "px";
-    });
-});
+    height = Math.floor(height);
+    document.getElementById(`ecp_${randomName}`).style.height = height + "px";
+}

I won't have access to the setHeight function in *.test.js. I tried to mark it as exported so that I can access it in a unit test, but that makes Java tests fail with

org.htmlunit.corejs.javascript.EvaluatorException: identifier is a reserved word: export (http://localhost:54289/jenkins/adjuncts/a326e762/org/biouno/unochoice/common/checkbox-content.js#9)

and with a lot of entries in the stack trace containing htmlunit.

Another option I thought of is moving the setHeight function to Util.ts. But it's a little different that other functions there. It doesn't return anything, but just modifies the state of a DOM element depending also on some data in another DOM element. Without any further refactoring and given its reliance on DOM can this even be tested?

[kinow]

Hi @yaroslavafenkin

I think it'd be easier to use Jest with an update to its current config that only loads typescript files (or you can write it in TS if you prefer), e.g. after applying your diff (thanks for that!!!):

diff --git a/package.json b/package.json
index 4b962b2..b0f4f2c 100644
--- a/package.json
+++ b/package.json
@@ -67,7 +67,8 @@
             "jest-junit"
         ],
         "testMatch": [
-            "<rootDir>/src/test/js/*.test.ts"
+            "<rootDir>/src/test/js/*.test.ts",
+            "<rootDir>/src/test/js/*.test.js"
         ]
     }
 }
diff --git a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
index 39d8add..21bb678 100644
--- a/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
+++ b/src/main/resources/org/biouno/unochoice/common/checkbox-content.js
@@ -2,18 +2,22 @@ window.addEventListener("DOMContentLoaded", () => {
     document.querySelectorAll(".checkbox-content-data-holder").forEach((dataHolder) => {
         const { maxCount, randomName } = dataHolder.dataset;
 
-        var height = 0;
-        var refElement = document.getElementById(`ecp_${randomName}_0`);
-        if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
-            for (var i = 0; i < maxCount; i++) {
-                height += refElement.offsetHeight + 3;
-            }
-        }
-        else {
-            height = maxCount * 25.5;
-        }
-
-        height = Math.floor(height);
-        document.getElementById(`ecp_${randomName}`).style.height = height + "px";
+        setHeight(maxCount, randomName);
     });
 });
+
+function setHeight(maxCount, randomName) {
+    var height = 0;
+    var refElement = document.getElementById(`ecp_${randomName}_0`);
+    if (maxCount > 0 && refElement && refElement.offsetHeight != 0) {
+        for (var i = 0; i < maxCount; i++) {
+            height += refElement.offsetHeight + 3;
+        }
+    }
+    else {
+        height = maxCount * 25.5;
+    }
+
+    height = Math.floor(height);
+    document.getElementById(`ecp_${randomName}`).style.height = height + "px";
+}

Then running yarn run test finds the extra test, and it passes successfully (although the test is not doing anything useful right now). I think we will have to use jsdom do prepare the elements for setHeight to be called, and then test a few scenarios 👍

Let me know if that helps.

Cheers

[yaroslavafenkin]

TS is fine for me to use.
I've created checkbox-content.test.ts in src/test/js. Here's its content (without imports, as those require tweaks depending on the approach taken):

describe('setHeight', () => {
    test('Sets the correct height on the element', () => {
        const randomName: string = "randomNameIndeed";
        const maxCount: number = 13;
        
        document.body.innerHTML = `<body><div id="ecp_${randomName}_0"></div><div id="ecp_${randomName}"></div></body>`;
        const target = document.querySelector(`#ecp_${randomName}`);
        
        expect(target.style.height).not.toBe("331px");
        setHeight(maxCount, randomName);
        expect(target.style.height).toBe("331px");
    });
});

Now there are few paths when it comes to code arrangement:

1. Leaving setHeight in checkbox-content.js. To be able to call the function in a test I've tried adding the export keyword before the function keyword in checkbox-content.js. Running npm run test results in log telling me setHeight is not defined.
2. I've tried to move the setHeight to Util.ts. With that I was able to write a test that passed just fine. I went to test whether everything still worked in the Jenkins UI, and it didn't. Util.js isn't included into the page as UnoChoice.js is. I could append Util.js somewhere here, then it would work. Would pollute the page with unneeded object though.
3. I've tried to move the setHeight to UnoChoice.es6 as that one is exported and included into the page already. I cannot import the setHeight from UnoChoice.es6 in a test though. First of all .es6 isn't a module extension recognized by jest. I was able to get past that by changing jest's config, but then I get the following error:

C:\projects\active-choices-plugin\src\main\resources\org\biouno\unochoice\stapler\unochoice\UnoChoice.es6:24
    import Util from './Util.ts';
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      1 | import {describe, test} from '@jest/globals';
    > 2 | import UnoChoice from '../../main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6';
        | ^
      3 | import expect from "expect";
      4 |
      5 | import jQuery from "jquery";