search

microsoft / component-detection

Scans your project to determine what components you use
MIT License
398 stars 83 forks source link

Add support for pnpm lock file format 6 #1028

Closed CraigMacomber closed 2 months ago

CraigMacomber commented 4 months ago

This addresses https://github.com/microsoft/component-detection/issues/503 by doing the following:

The old code wasn't clear about what file formats it actually worked with, and a lot of its tests seems to be from versions earlier than 5 which it didn't really parse correctly (ex: the version it parses isn't present in those versions as it had a different name). Thus to preserve existing behavior I pointed cases which failed to detect a version through the old V5 code.

The included verification tests didn't cover a lot of cases (link and file references, aliased packages, shared shrink-wrap format, disambiguated deps with peer deps) so I also tested on the package lock from https://github.com/microsoft/FluidFramework which is 44k lines and has all the mentioned edge cases. If desired, I can add the v5 and v6 versions of that file to the verification tests directory.

This should be in a working state but I need to actually understand how the verification tests results are verifies before this should be merged.

This is my first time working in this codebase, as well as the first time in several years that I'm working in C#, so please be extra careful reviewing this: don't trust that I know what I'm doing.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 81.55340% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 75.4%. Comparing base (c06abda) to head (e8f2072). Report is 21 commits behind head on main.

Files Patch % Lines
...tDetection.Detectors/pnpm/PnpmComponentDetector.cs 72.8% 11 Missing and 5 partials :warning:
...ntDetection.Detectors/pnpm/PnpmParsingUtilities.cs 91.6% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1028 +/- ## ===================================== Coverage 75.3% 75.4% ===================================== Files 237 241 +4 Lines 10423 10512 +89 Branches 1043 1054 +11 ===================================== + Hits 7858 7927 +69 - Misses 2272 2286 +14 - Partials 293 299 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

CraigMacomber commented 4 months ago

It appears that the VS code dev container setup doesn't include PowerShell so I can't run the VerificationTests.ps1 script locally. The instructions in there also make explicit reference to visual studio. It also says "The automation script above will output variables that needs to be replaced in ComponentDetectionIntegrationTests.cs file.": as far as I can tell thats not accurate as it looks like that file pulls in the data from environment variables.

Anyway its unclear to me what the workflow is for adding more test cases to the verification script. I'd like to include my added v6 file works but I don't know how.

CraigMacomber commented 4 months ago

Looking at the code coverage, I determined I could use a test for the shared-shrinkwrap case that workspaces use, and one for renamed/aliased dependencies, so I have added both. More testing is possible (ex: file and link cases, peer deps etc.) but I think I'm past the point of diminishing returns.

CraigMacomber commented 3 months ago

@microsoft/ose-component-detection-maintainers : I'd like assistance dealing with the snapshot tests as noted in https://github.com/microsoft/component-detection/pull/1028#issuecomment-1992470325. If this is not practical to get assistance on, I can revert the extended snapshot test coverage which should get this passing on CI: maybe that would get this reviewed sooner?

cobya commented 2 months ago

It appears that the VS code dev container setup doesn't include PowerShell so I can't run the VerificationTests.ps1 script locally. The instructions in there also make explicit reference to visual studio. It also says "The automation script above will output variables that needs to be replaced in ComponentDetectionIntegrationTests.cs file.": as far as I can tell thats not accurate as it looks like that file pulls in the data from environment variables.

Anyway its unclear to me what the workflow is for adding more test cases to the verification script. I'd like to include my added v6 file works but I don't know how.

I'll follow up internally here as the verification CI tests use a private repo which we can add the additional test file to.

cobya commented 2 months ago

@microsoft/ose-component-detection-maintainers : I'd like assistance dealing with the snapshot tests as noted in #1028 (comment). If this is not practical to get assistance on, I can revert the extended snapshot test coverage which should get this passing on CI: maybe that would get this reviewed sooner?

When adding new components like this, the verification test failures are expected as it is performing a diff based on the last "known good version" so adding new files will trip the failure. I'll verify before merging but you shouldn't need to worry about this for now.

cobya commented 2 months ago

Closing this as #1110 has been opened with the related experiments changes.