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

Add methods to configure context lines for fingerprinting #1943

Open
wants to merge 4 commits into
base: 13.0.0
Choose a base branch
from

Conversation

Jay-oao
Copy link

@Jay-oao Jay-oao commented Feb 3, 2025

This PR introduces a tool-based configuration for setting the context lines considered during the fingerprinting process in FullTextFingerprint.
See :
https://issues.jenkins.io/browse/JENKINS-61607
jenkinsci/analysis-model#1136
jenkinsci/analysis-model-api-plugin#184

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

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

The UI elements are missing:

(There is also a test missing but this is optional here as this requires more work than the actual feature.)

@@ -365,7 +372,13 @@ private void createFingerprints(final Report report) {
report.logInfo("Creating fingerprints for all affected code blocks to track issues over different builds");

FingerprintGenerator generator = new FingerprintGenerator();
generator.run(new FullTextFingerprint(), report, getCharset());
if (linesLookAhead < 0) {
//Empty constructor defaults context lines to 3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//Empty constructor defaults context lines to 3
// no-arg constructor defaults context lines to 3

@uhafner
Copy link
Member

uhafner commented Feb 3, 2025

The broken tests are not you fault: somehow the Java 17 Jenkins release does not work with my docker tests right now. I am still trying to understand what is going on here...

@Jay-oao
Copy link
Author

Jay-oao commented Feb 5, 2025

The UI elements are missing:

(There is also a test missing but this is optional here as this requires more work than the actual feature.)

I'd be happy to help if you could guide me on the steps to get started with the tests.

@@ -35,6 +35,11 @@
<f:combobox/>
</f:entry>

<f:entry title="${%title.configureContextLines}" field="linesLookAhead"
help="${descriptor.getHelpFile('contextLines')}">
Copy link
Member

Choose a reason for hiding this comment

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

I think we should conform to the default naming scheme

Suggested change
help="${descriptor.getHelpFile('contextLines')}">
">

@@ -0,0 +1,6 @@
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Can you move that to help-linesLookAhead.html, then we do not need to specify the name in the entry.

@uhafner
Copy link
Member

uhafner commented Feb 5, 2025

(There is also a test missing but this is optional here as this requires more work than the actual feature.)

I'd be happy to help if you could guide me on the steps to get started with the tests.

For a test a good staring point are the test cases in AffectedFilesResolverITest: these tests verify that the affected files are read and processed. Maybe we can create a new test that extends this functionality?

Currently theses test have one build to show a warning in a file. What we need is: two builds where the warning is moved in the file by e.g. one line in the second build (so we need the same source code file slightly modified by inserting a new line somewhere before). This warning should be marked as existing warning based on a small value of the fingerprint. But when the changed line is included in the fingerprint then the warning should still be marked as existing.

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