reduce code duplication in unit tests #975

Activity

1. erwindon

   self-assigned this

   on Sep 16, 2026
2. erwindon

   added

   enhancement

   on Sep 16, 2026
3. erwindon commented on Sep 16, 2026

   

   erwindon

   on Sep 16, 2026

   OwnerAuthor

   More actions

   @rawsun007 FYI
4. rawsun007 commented on Sep 16, 2026

   

   rawsun007

   on Sep 16, 2026

   More actions

   Written by Claude Opus 5 in Claude Code, posting through @rawsun007's account.

   Thanks for the pointer, and understood on #964 — your #966 design covers more than that patch did, so not merging it was the right call.

   Not asking to take this one, you have it assigned. But the Sonar numbers say deduplicating the tests cannot clear the gate on #974 by itself, so it seemed worth mentioning before you spend more passes on it.

   Asking the API for the per-file split on #974:

   	new lines	duplicated
   tests/unit/ParseCommandLine.test.js	294	62.9%
   tests/unit/CommandBox.test.js	196	56.6%
   saltgui/static/scripts/CommandBox.js	428	8.4%
   total	1100	30.2%

   That is 296 duplicated lines in the tests and 36 in CommandBox.js. Since the metric is duplicated / new, the 36 alone are 3.3% of 1100 — over the 3% threshold — and deduplicating the tests shrinks the denominator, so it gets worse as that work succeeds. That is likely why the three attempts stopped at 11.9%.

   The 36 are one pair, CommandBox.js:1021-1038 and 1047-1064:

   if (pValidationResult.errors.length === 0 && pValidationResult.warnings.length === 0) {
     indicatorElement.textContent = "";
     Utils.addToolTip(indicatorElement, "");
     return;
   }
   const hasErrors = pValidationResult.errors.length > 0;
   ...

   It is actually three copies, not two — _displayCmdValidationIndicator, _displayTargetValidationIndicator and _displayFormValidationIndicator differ only in the element id, plus a null guard in the Form one. Sonar counts only the last two because the Cmd copy happens to compute tooltipText before assigning textContent instead of after, so the token sequence differs. Same code, different order.

   One helper taking the element id would remove all three copies and shrink new lines at the same time.

   Happy to leave the rest to you.
5. rawsun007 commented on Sep 16, 2026

   

   rawsun007

   on Sep 16, 2026

   More actions

   Written by Claude Opus 5 in Claude Code, posting through @rawsun007's account.

   Done, on top of your b6c93e9: rawsun007/SaltGUI branch table-driven-parsecommandline, one commit 96395c2. Not opening a PR since this is your branch to take or leave — cherry-pick it, or say the word and I will open one against #974.

   ParseCommandLine.test.js goes 726 lines to 198. Same inputs, same expectations, as two tables and one runner:

   const parse = (pCommandLine) => {
     const tokens = [];
     const args = [];
     const params = {};
     const result = ParseCommandLine.parseCommandLine(pCommandLine, tokens, args, params);
     return {"args": args, "params": params, "result": result, "tokens": tokens};
   };

   OK_CASES rows are [commandLine, expectedArgs, expectedParams] compared with deepEqual, which pins the arg and param counts the old code asserted separately. ERROR_CASES rows are [commandLine, messagePrefix, messageFragment]. The section comments (// OCTAL, // SEXAGESIMAL, ...) are kept as comments between the rows.

   76 tests instead of 1, because each row is its own it() — a failure now names the command line rather than stopping one large test at its first bad assertion. Full unit suite 401 passing, eslint clean. Mutation check, to confirm the table still bites:

   broken in ParseCommandLine.js	result
   octal radix 8 to 10	3 failing
   hex radix 16 to 10	3 failing
   binary radix 2 to 10	3 failing
   drop None from the null pattern	1 failing

   I have not run SonarCloud, so I am not claiming a number — the repeated 14-line block is gone, and your CI is the judge of the density.

   Two things I found while transcribing, both left exactly as they are on your branch:

   • ParseCommandLine.test.js:160 calls parseCommandLine("[1,2", args, params) with three arguments, so args lands in the tokens slot and params is undefined. The case only asserts result.startsWith("No valid array found,"), so it passes either way. The rewrite calls it with four.
   • the "NONE" case has the // GENERAL WHITESPACE HANDLING comment sitting between the call and its assertions.

   On assert.equal(String(args[0]), nr, nr) — that was mine, in #964, not in your tree. SonarCloud flagged it there under "Replace this argument or its duplicate", because nr was both the expected value and the failure message. Mentioned it only as the other thing that gate caught me on.

   And the reason I raised the 36 lines in CommandBox.js separately: _displayCmdValidationIndicator, _displayTargetValidationIndicator and _displayFormValidationIndicator are three copies of the same 18 lines, differing in the element id plus a null guard in the Form one. Sonar only counts two of the three, because the Cmd copy computes tooltipText before assigning textContent instead of after.
6. rawsun007 commented on Sep 16, 2026

   

   rawsun007

   on Sep 16, 2026

   More actions

   Written by Claude Opus 5 in Claude Code, posting through @rawsun007's account.

   Rebased onto your current head (88b90f2) and force-pushed — rawsun007/SaltGUI branch table-driven-parsecommandline, now 26c755a. The earlier commit no longer applied, because "improved some validation messages" changed one of the cases it asserts:

   Valid dictionary, but followed by text\n   + "in: {"a}":1"
   Valid dictionary, but followed by extra text\n   + "dictionary: {"a}":1}" + "extra: }"
   

   That case now wants two fragments, so error rows carry a list instead of a single fragment. 76 tests pass, full unit suite 401, eslint clean, and the mutation check still bites — octal and hex radix 3 failing each, the new message text 1, dropping None from the null pattern 1.

   Cherry-picks cleanly onto 88b90f2.