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

Support deterministic Cobertuna Reports #121

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Kuntal271
Copy link

@Kuntal271 Kuntal271 commented Sep 7, 2024

Description

This pull request updates the file path handling in the Coverage plugin to support deterministic builds, addressing issues with paths starting with "/_/". The change ensures compatibility with Coverlet's output format, which uses these paths for deterministic reporting. This adjustment allows the plugin to properly locate and process source files, fixing the problem of missing sources in the coverage reports.

Note: This is a work in progress (WIP) PR. The implementation is still being finalized, and additional testing and adjustments are ongoing.

Testing Done

  • Automated Tests:

    • Added unit tests to verify that paths starting with "/_/" are correctly transformed and handled by the plugin.
    • Ensured that the plugin’s coverage report generation accurately reflects the modified path handling logic.
  • Manual Testing:

    • Tested with various coverage reports generated by Coverlet to confirm that the plugin now properly recognizes and processes files with deterministic paths.
    • Verified that the plugin’s output matches expected results, with no missing source files.

Submitter Checklist

  • Branch: This PR is from a feature branch and not the main branch.
  • Title: The PR title accurately reflects the change.
  • Description: Provided a clear description of the changes made.
  • Linked Issues: Related to Coverlet's deterministic path issue (JENKINS-73635).
  • Linked PRs: No upstream/downstream PRs directly linked, but relevant to Coverlet integration.
  • Tests Provided: Included both automated tests for the new path handling logic and manual testing scenarios to ensure functionality.

@uhafner uhafner added the enhancement Enhancement of existing functionality label Sep 8, 2024
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.

Please also add a small test case that shows that your code actually works.


return packageNode.findOrCreateFileNode(getFileName(fileName), path);
var relativePath = PATH_UTIL.getRelativePath(fileName);
var actualPath = relativePath.startsWith("/_/") ?
Copy link
Member

Choose a reason for hiding this comment

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

Extract "/_/" as a constant with a meaningful name (deterministic...).

Copy link
Member

Choose a reason for hiding this comment

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

can't you use String#replaceFirst here?

Copy link
Author

Choose a reason for hiding this comment

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

Good point! I've replaced the manual prefix check with String#replaceFirst to simplify the logic

var actualFileName = (fileName != null && fileName.toString().startsWith("/_/")) ?
fileName.toString().replace("/_/", "./") :
(fileName != null ? fileName.toString() : relativePath);
return actualFileName;
Copy link
Member

Choose a reason for hiding this comment

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

Please extract the file name logic into a method and reuse it here.

Copy link
Author

Choose a reason for hiding this comment

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

👍

@theKBro
Copy link

theKBro commented Nov 12, 2024

whats the status here? is this abandoned?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants