Refactor license scanning related specs
Summary
During Implement License Scanning SBOM scanner (!109447 - merged) and Fetch latest license scanning build from `Licen... (!109723 - merged), a lot of license scanning specs were updated to test the context when the license_scanning_sbom_scanner
feature flag was enabled. The main focus during this time was to ship the SBoM scanner feature, so the tests duplicated a lot of code to quickly test the feature. This issue covers the areas where we can improve on the testing structure.
@adamcohen and @fcatteau discussed the following improvements to be done during the review of the MR to add the SBoM based license scanning FF.
When testing the
SbomScanner
, we need to create matchingpm_package
DB entries, for example in this spec. This couples the specs tightly with the gl-sbom.cdx.json.gz fixture file that contains these components, and since this fixture is a gzipped archive, it's difficult to update as well as view the contents.It would be better if we could automatically create these
pm_package
DB entries when using theee_ci_pipeline
factory to create a pipeline using thewith_cyclonedx_report
trait.It would also be good if we could provide a
gl-sbom.cdx.json
file in plaintext, rather than a gzipped archive.Many of the existing specs have duplicated setup and repetitive blocks, as well as using
let
instead oflet_it_be
orsome_record.reload
instead of usinglet_it_be_with_reload
.The
let
variables are sometimes too far away from where they're overriddenSome of the example descriptions are incorrect, misleading or need to be updated. There's also a lot of "specify" blocks without any description that makes it difficult to understand the true intent. It would be better to replace these with more verbose
it "does something"
examples to better explain the expectationSome factories use random values, such as this one. This makes the tests non-deterministic and makes the specs even more difficult to follow, since the values we're asserting on can’t be hardcoded and must be made dynamic.
Randomness in specs can lead to very difficult to track down random CI failures, it addition to making the specs hard to understand.
Improvements
- Condensed specs are easier to understand.
- Easier to search for FF related contexts when cleaning up.
- Improved spec performance which leads to faster pipelines.
- Clear ownership of license compliance code.
Risks
- Increased time spent in setting up the components when using the
:with_cyclonedx_report
trait in the:ee_ci_pipeline
factory.- This can be mitigated by using
let_it_be
in place oflet
andbuild
instead ofcreate
wherever possible.
- This can be mitigated by using
- Missing test coverage
- This can be detected via the test coverage widget in the MR(s). Ideally, we should increase test coverage if possible.
Involved components
The following components are impacted:
- ee/spec/controllers/projects/dependencies_controller_spec.rb
- ee/spec/controllers/projects/licenses_controller_spec.rb
- ee/spec/controllers/projects/merge_requests_controller_spec.rb
- ee/spec/controllers/projects/pipelines_controller_spec.rb
- ee/spec/features/projects/licenses/maintainer_views_policies_spec.rb
- ee/spec/features/projects/pipelines/pipeline_spec.rb
- ee/spec/lib/gitlab/license_scanning_spec.rb
- ee/spec/models/approval_merge_request_rule_spec.rb
- ee/spec/models/ci/pipeline_spec.rb
- ee/spec/models/merge_request_spec.rb
- ee/spec/models/sca/license_compliance_spec.rb
- ee/spec/requests/api/dependencies_spec.rb
- ee/spec/serializers/licenses_list_entity_spec.rb
- ee/spec/services/ci/compare_license_scanning_reports_collapsed_service_spec.rb
- ee/spec/services/ci/compare_license_scanning_reports_service_spec.rb
- ee/spec/workers/refresh_license_compliance_checks_worker_spec.rb
- ee/spec/lib/gitlab/license_scanning/artifact_scanner_spec.rb
Implementation plan
-
Create associated :pm_packages
when creating cyclonedx reports in pipeline. -
Reorganize contexts by required pipeline for the feature flag value e.g. a license_scanning
report whenlicense_scanning_sbom_scanner
is disabled. See #390093 (comment 1264196383). -
Use the conventional context setup of when <feature_flag> is disabled/enabled
when testing the on and off states of the FF. See #390093 (comment 1264188948) -
Use let_it_be
orlet_it_be_with_reload
to improve performance of test setup. -
Reorganize cyclonedx traits to specify what components they have inside. Ex: with_cyclonedx_report
->with_cyclonedx_for_gem_go_npm
. See !109723 (comment 1268379304) -
Add the feature category for all specs and examples related to license compliance. This makes the related specs easier to find in the future.
-
ee/spec/controllers/projects/merge_requests_controller_spec.rb -
Add a context when a merge request has a license scanning report
to make the scenario explicit. See !109723 (comment 1259773826). -
Add a context when a merge request has a sbom report
to make the scenario explicit. See !109723 (comment 1259774073).
-
-
ee/spec/features/projects/licenses/maintainer_views_policies_spec.rb -
Create packages only when a sbom report exists. See !109723 (comment 1259779977). -
Separate when a pipeline exists
context into two contexts. See !109723 (comment 1259778032): when the pipeline has a sbom and when the pipeline has a license scanning report. -
!109723 (comment 1256820722)
-
-
ee/spec/features/projects/pipelines/pipeline_spec.rb -
Use license scanning report
andsbom report
instead ofLicense Compliance artifact
andCycloneDX artifact
for consistency across tests.
-
-
ee/spec/services/ci/compare_license_scanning_reports_collapsed_service_spec.rb -
Created a shared example for an empty report. See !109723 (comment 1271043131).
-
-
ee/spec/lib/gitlab/license_scanning/artifact_scanner_spec.rb -
Test the same contexts as sbom_scanner_spec.rb
-
Optional: Intended side effects
- The specs are simplified which lessens the cost of maintenance.