Commit 5e0ff12
authored
ci(lint): expand jscpd to use branch comparison #5862
## Problem
Follow up to: #5833
A static threshold is useful, but if the threshold fails, we want to be
able to see what the new duplicated code is.
## Solution
Run `jscpd` on the current branch, and if it is above the set threshold,
we fail (same as before).
Now, we also filter the resulting report to contain only clones from the
files changed with respect to the base branch. Then, if the filtered
report is non-empty, i.e. one of the changed files has a clone, fail and
display the clones in CI.
Adding this extra step allows us to know when we are adding a clone that
was not otherwise in the codebase. It also allows us to see this clone
in CI, before the PR is merged, giving the developer a chance to remove
it.
As an example, see commit
(a8e4860)
where a copy-pasted function is added, and notice that CI fails and
displays only the section of the report relevant to this clone. Then,
once we remove this work in the following commit
(53d3973),
the check passes.
Included in this PR, is that it runs only on `pull_request` and no
longer on `push`.
## Notes
- In developing this, it became apparent that `jscpd` is not perfect and
was not able to detect the clone added in
(e7357c0).
Will need to dig deeper into their algorithm (based on
https://en.wikipedia.org/wiki/Rabin%E2%80%93Karp_algorithm) and
implementation (https://github.com/kucherenko/jscpdto) to understand why
this is missed, and if it presents a larger problem.
- Also, if a clone already exists in a file that is changed in the PR,
this check will fail. In this case, the options are to ignore the check,
mark the case as an exception, or fix the clone. To encourage developers
to not repeatedly ignore the check, we still employ a static threshold
via #5833.
- If the branch hasn't been rebased up to date, it can pick up clones
from unrelated changes.
## Future Work
- We want to lower the threshold as much as possible by removing
duplicates and marking exceptions when necessary.
- More experience using this tool will help us find a `minLines`
argument that provides the desired sensitivity. This argument allows us
to ignore clones under `minLines` in length. Right now it is 3, which is
likely too granular, but better to start there and loosen as needed.
- Right now the CI output of the clones is very verbose. Digging through
this could be time consuming, especially when many clones are added.
Could be worth filtering it to provide a more ergonomic experience.1 parent a4662aa commit 5e0ff12
File tree
3 files changed
+91
-21
lines changed- .github/workflows
3 files changed
+91
-21
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
| 3 | + | |
4 | 4 | | |
5 | | - | |
6 | | - | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
7 | 9 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | 58 | | |
77 | 59 | | |
78 | 60 | | |
| |||
0 commit comments