build: break dependency so tests can be more reliable #8212
Conversation
|
I fixed them in #8205 FWIW |
|
Doesn't really need a seam IMHO given the actual expiry date is not relevant to the test. |
| * | ||
| * @param now represents current time. Used to compare suppression expiration dates set within the 'until' attribute. | ||
| */ | ||
| SuppressionHandler(Calendar now) { |
There was a problem hiding this comment.
Can we use something more modern like an Instant or ZonedDateTime (if TZ dependent) in API or does that require too many other changes/introduce inconsistency with existing public APIs already taking Calendars (🤮)?
There was a problem hiding this comment.
java.util.Calendar seems to be exposed in our API in a limited amount of places, in the core, through org.owasp.dependencycheck.xml.suppression.SuppressionRule and org.owasp.dependencycheck.utils.DateUtil.
Removing these public elements will technically require a breaking change of the core at some point but we could start by providing alternative APIs and deprecating the old ones for a removal in the next major.
There was a problem hiding this comment.
Yeah, I am not sure how "seriously" we treat Java level visibility on these core APIs, especially given java's limited ability to really enforce visibility (short of the module system).
Do you know how many projects there are which depend on the core which are not official or "close" plugins (e.g jenkins, azure etc). I wonder if it is sufficient that we check we're not breaking the major ones when making changes to the core.
There was a problem hiding this comment.
There's some adjacent discussion at dependency-check/dependency-check-gradle#491 (comment) where I express my preference that things like the Settings API are not directly exposed to end users via things like the Gradle plugin - but where your opinion would probably be valuable.
|
Closing following the merge of #8205 |
if you are keen to rebase, there is still some value to your change IMHO in terms of improving ability to test pieces - but your call. |
Description of Change
Following the new year, 2 tests of the
SuppressionParserTestclass are failing, making all new pipeline jobs failing (see: https://github.com/dependency-check/DependencyCheck/actions/runs/20669128797/job/59346855943?pr=8192).Instead of extending the date in the suppression xml documents, this change suggests to introduce a seam, without changing the public API, in order to make the test independent of the system clock, preventing tests to eventually fail.
This change is just a refactoring, not changing the current application behavior.
Related issues
Have test cases been added to cover the new functionality?
no, but existing test cases have been updated