FossIdTest improvements #5797

mnonnenmacher · 2022-09-14T16:34:56Z

See the commit messages for details. This is preparation for removing the legacy scanner.

codecov · 2022-09-14T16:53:32Z

Codecov Report

Merging #5797 (4048e7f) into main (4048e7f) will not change coverage.
The diff coverage is n/a.

❗ Current head 4048e7f differs from pull request most recent head e13b6e6. Consider uploading reports for the commit e13b6e6 to get more accurate results

@@            Coverage Diff            @@
##               main    #5797   +/-   ##
=========================================
  Coverage     57.71%   57.71%           
  Complexity     2190     2190           
=========================================
  Files           321      321           
  Lines         18989    18989           
  Branches       3730     3730           
=========================================
  Hits          10960    10960           
  Misses         6887     6887           
  Partials       1142     1142

Flag	Coverage Δ
funTest-analyzer-docker	`74.54% <0.00%> (ø)`
funTest-non-analyzer	`46.21% <0.00%> (ø)`
test	`27.56% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sschuberth · 2022-09-14T19:19:50Z

scanner/src/test/kotlin/scanners/fossid/FossIdTest.kt

@@ -676,9 +676,10 @@ class FossIdTest : WordSpec({

            val fossId = createFossId(config)

-            val scannerRun = fossId.scan(listOf(pkg1, pkg2))
+            fossId.scan(listOf(pkg1))


Does this line then need to be kept at all? I currently don't see an assertion that would depend on it.

How I understand the test the last check coVerify { service.deleteScan(USER, API_KEY, scanCode) } verifies that the scan of pkg1 gets deleted as well if the scan of pkg2 fails. But to be honest, I had a hard time coming to that conclusion and the whole test class seems a bit too complex.

@sschuberth this is required by scannerRun.results.scanResults.keys shouldHaveSize 1

If one scan fails, other successful scans should be deleted hence there should be no result for them.

That being said, I think theses changes are wrong @mnonnenmacher : now there a two scanner runs.
The first one is successful. The second one fails. However since these are two separate runs, the first scan is not deleted anymore.

The test does not fail which shows the other scan is deleted. The reason is that the same instance of FossId is used and createdScans (which is used as input when deleting scans) is a class variable. However, if the scans would be triggered the other way round it would probably not work. It seems an interface change is required, but I'm also a bit surprised that this comes up only now since it's running like this for several months now on our side.

the PackageScannerWrapper interface needs to be adapted to take a list of packages

I was wondering whether that doesn't make sense anyway, as it could allow for some significant scanner-implementation-internal performance improvements. A bit similar to how I changed PackageCurationProvider to take collection of packages in 0592a68.

@nnobelis So I had a look at the current implementation, especially these two places:

ort/scanner/src/main/kotlin/scanners/fossid/FossId.kt

Lines 314 to 318 in 1258f27

val scanCode = if (config.deltaScans) {

checkAndCreateDeltaScan(scans, url, revision, projectCode, projectName)

} else {

checkAndCreateScan(scans, url, revision, projectCode, projectName)

}

ort/scanner/src/main/kotlin/scanners/fossid/FossId.kt

Lines 351 to 355 in 1258f27

if (!config.keepFailedScans) {

createdScans.forEach { code ->

logger.warn { "Deleting scan '$code' during exception cleanup." }

deleteScan(code)

}

Both are inside a loop over the packages to scan, so if I read the code correctly the behavior was always like this, that scans started before a failed scan would be deleted, but other scans could still be started after a failed scan. Is this an issue that needs to be fixed or is the current code working correctly?

Yes I think you find a bug. Originally, the logic was to delete the scan in the catch and let the exception bubble up. Then somebody came with "it should not throw an exception, it should create an ortIssue instead.

But you are right, if a scan fails all successful scans should be deleted (if the option is set !) and no further scan should be created.
If the option is not set, successful scans should be kept and further scans should be run.
Makes sense ?

Ok, I will propose a fix in a separate PR and update this PR afterwards.

@nnobelis For now I have only added a commit that changes the test to illustrate the issue which also shows that this refactoring does not change the behavior.

the PackageScannerWrapper interface needs to be adapted to take a list of packages

I was wondering whether that doesn't make sense anyway, as it could allow for some significant scanner-implementation-internal performance improvements. A bit similar to how I changed PackageCurationProvider to take collection of packages in 0592a68.

This might be, but this requires changes in ExperimentalScanner and because our only PackageScannerWrapper implementation FossId does not do any such optimizations I would not want to work on this before we have an actual use case.

scanner/src/test/kotlin/scanners/fossid/FossIdTest.kt

nnobelis · 2022-09-15T05:24:08Z

scanner/src/test/kotlin/scanners/fossid/FossIdTest.kt

@@ -676,9 +676,10 @@ class FossIdTest : WordSpec({

            val fossId = createFossId(config)

-            val scannerRun = fossId.scan(listOf(pkg1, pkg2))
+            fossId.scan(listOf(pkg1))


That being said, I think theses changes are wrong @mnonnenmacher : now there a two scanner runs.
The first one is successful. The second one fails. However since these are two separate runs, the first scan is not deleted anymore.

nnobelis · 2022-09-15T05:27:08Z

Please see my comment, I think changing the signature of scan is wrong here, because it removes the current feature of deleting all scans if one fails.

If `keepFailedScans` is set to `false`, all scans should be deleted if a single scan fails. Change `FossIdTest` to show that currently scans started after a failed scan are not deleted. Signed-off-by: Martin Nonnenmacher <[email protected]>

Refactor the test that tests the deletion of a failed scan to call the scan function separately for the two packages. This is a preparation for changing the scan function to take only one package instead of a list. Calling the scan functions for the packages separately works because `FossId` stores the relevant information for deleting scans in class properties. Signed-off-by: Martin Nonnenmacher <[email protected]>

Change the scan function to take a single package instead of a list to prepare an upcoming refactoring where `FossId` will be changed to implement only the `PackageScannerWrapper` interface which scans packages individually. Signed-off-by: Martin Nonnenmacher <[email protected]>

Run the `FossId.scanPackages()` directly instead of through the `Scanner` class. This simplifies the test code and eases the migration of the `FossId` class to support only the experimental scanner. Signed-off-by: Martin Nonnenmacher <[email protected]>

mnonnenmacher · 2022-09-19T11:16:27Z

@sschuberth @nnobelis Would one of you mind approving this or is there something left to do?

mnonnenmacher requested a review from a team as a code owner September 14, 2022 16:34

sschuberth reviewed Sep 14, 2022

View reviewed changes

mnonnenmacher force-pushed the improve-fossid-test branch from 23b8644 to fc1aeec Compare September 14, 2022 20:37

nnobelis requested changes Sep 15, 2022

View reviewed changes

mnonnenmacher added 4 commits September 18, 2022 12:50

mnonnenmacher force-pushed the improve-fossid-test branch from fc1aeec to e13b6e6 Compare September 18, 2022 10:55

mnonnenmacher requested review from sschuberth and nnobelis September 18, 2022 11:00

sschuberth assigned nnobelis Sep 18, 2022

sschuberth requested a review from a team September 18, 2022 17:09

nnobelis approved these changes Sep 19, 2022

View reviewed changes

mnonnenmacher merged commit 425ac7c into main Sep 19, 2022

mnonnenmacher deleted the improve-fossid-test branch September 19, 2022 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FossIdTest improvements #5797

FossIdTest improvements #5797

mnonnenmacher commented Sep 14, 2022

codecov bot commented Sep 14, 2022 •

edited

Loading

sschuberth Sep 14, 2022

mnonnenmacher Sep 14, 2022

nnobelis Sep 15, 2022

nnobelis Sep 15, 2022

mnonnenmacher Sep 15, 2022

sschuberth Sep 15, 2022

nnobelis Sep 15, 2022

mnonnenmacher Sep 15, 2022

mnonnenmacher Sep 18, 2022

mnonnenmacher Sep 18, 2022

nnobelis Sep 15, 2022

nnobelis commented Sep 15, 2022

mnonnenmacher commented Sep 19, 2022

	val scanCode = if (config.deltaScans) {
	checkAndCreateDeltaScan(scans, url, revision, projectCode, projectName)
	} else {
	checkAndCreateScan(scans, url, revision, projectCode, projectName)
	}

	if (!config.keepFailedScans) {
	createdScans.forEach { code ->
	logger.warn { "Deleting scan '$code' during exception cleanup." }
	deleteScan(code)
	}

FossIdTest improvements #5797

FossIdTest improvements #5797

Conversation

mnonnenmacher commented Sep 14, 2022

codecov bot commented Sep 14, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnobelis commented Sep 15, 2022

mnonnenmacher commented Sep 19, 2022

codecov bot commented Sep 14, 2022 •

edited

Loading