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

ci(lint): expand jscpd to use branch comparison #5862

Merged
merged 19 commits into from
Nov 5, 2024

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Oct 24, 2024

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

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.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@Hweinstock Hweinstock changed the title ci(lint): branch comparison idea [WIP] ci(lint): expand jscpd to use branch comparison [WIP] Oct 25, 2024
@Hweinstock Hweinstock marked this pull request as ready for review October 25, 2024 21:03
@Hweinstock Hweinstock requested a review from a team as a code owner October 25, 2024 21:03
@Hweinstock Hweinstock changed the title ci(lint): expand jscpd to use branch comparison [WIP] ci(lint): expand jscpd to use branch comparison Oct 25, 2024
@justinmk3 justinmk3 merged commit 5e0ff12 into aws:master Nov 5, 2024
10 of 11 checks passed
@@ -1,7 +1,9 @@
{
"pattern": "packages/**/*.ts",
"ignore": ["**node_modules**", "**dist**"],
"ignore": ["**node_modules**", "**dist**", "*/scripts/*"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be **/scripts/**

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, verified you are correct. Here's the followup: #5930

@Hweinstock Hweinstock deleted the lint/noCopyPaste2 branch November 5, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants