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

test: replace mock with memory cache and fix non-deterministic tests #8410

Merged
merged 6 commits into from
Feb 18, 2025

Conversation

knqyf263
Copy link
Collaborator

Overview

This PR refactors scanner tests to use memory cache instead of mocks, which revealed the need for consistent sorting of artifact details.

Description

Scanner Test Refactoring

  • Replaces mock applier with memory cache for more realistic testing
  • Removes redundant test expectations and simplifies test cases
  • Discovered that test results were non-deterministic due to undefined ordering of collections returned by ApplyLayers

Artifact Details Sorting (to fix non-deterministic tests)

To address the non-deterministic test results discovered during the refactoring, added sorting capabilities:

  • Implements Sort() method for ArtifactDetail to ensure consistent ordering
  • Adds sorting methods for collections:
    • Applications: by type, file path, and package count
    • Secrets: by file path
    • LicenseFiles: by type and file path
  • Calls Sort() after ApplyLayers to guarantee deterministic results

The addition of sorting was necessary because the previous mock-based tests were masking the undefined ordering of collections in the actual implementation.

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

This commit updates the local scanner tests to use a memory cache instead of mocking the applier. The changes include:
- Removing mock applier files
- Updating test cases to use `setupCache` function
- Simplifying test setup and improving test readability
- Adding package identifier details to test packages
This commit introduces sorting capabilities for various collections in the artifact details:
- Added sort methods for Applications, Secrets, and LicenseFiles
- Implemented a Sort() method for ArtifactDetail to consistently sort different collections
- Updated types to support sorting based on specific criteria like type, file path, and package count
@knqyf263 knqyf263 self-assigned this Feb 17, 2025
@knqyf263 knqyf263 changed the title test(scanner): replace mock with memory cache and fix non-deterministic tests test: replace mock with memory cache and fix non-deterministic tests Feb 17, 2025
Modify CycloneDX JSON test data:
- Remove license entry for pom.xml
- Add empty license text fields for Java package licenses
- Reorder license entries in the JSON structure
Change the wantApps variable type from []types.Application to types.Applications to align with recent sorting and collection updates
@knqyf263 knqyf263 marked this pull request as ready for review February 17, 2025 17:35
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

@knqyf263 LGTM
left couple small comments.

python39min,
{
FilePath: "usr/lib/python/site-packages/urllib3-3.2.1/METADATA",
Packages: []ftypes.Package{urllib3Pkg},
Copy link
Contributor

Choose a reason for hiding this comment

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

Previous test included python39min:

Add this package here or remove this variable (because it is not used):

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this file doesn't have changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's sorted now.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i understood how the new sorting affected.
Sorry for disturbing you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem. Thanks for your comment. I've removed unneeded fields.
1f71820

Remove empty license text fields for package licenses in CycloneDX JSON test data
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

LGTM

@DmitriyLewen DmitriyLewen added this pull request to the merge queue Feb 18, 2025
Merged via the queue into aquasecurity:main with commit 10b8127 Feb 18, 2025
12 checks passed
@knqyf263 knqyf263 deleted the test/use_mem_cache branch February 18, 2025 06:24
@knqyf263 knqyf263 mentioned this pull request Feb 18, 2025
11 tasks
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