Skip to content

Set Order of Package Files in Package Files API Pagination Spec

Jason Goodman requested to merge fix-order-in-package-files-spec into master

What does this MR do and why?

Set order of package files in package files API pagination spec.

This prevents flaky spec issues caused by the order of the package files being nondeterministic.

For example, this MR: !146956 (merged)

Has a failing pipeline: https://gitlab.com/gitlab-org/gitlab/-/pipelines/1221263671

Because this spec is failing in this job: https://gitlab.com/gitlab-org/gitlab-foss/-/jobs/6441513723

Failures:
  1) API::PackageFiles GET /projects/:id/packages/:package_id/package_files without the need for a license with pagination params when viewing the first page returns first 2 packages
     Failure/Error: expect(json_response.map { |item| item['id'] }).to eq(items.flatten)
       expected: [292, 290]
            got: [290, 291]
       (compared using ==)
     # ./spec/support/helpers/api_helpers.rb:67:in `expect_paginated_array_response'
     # ./spec/requests/api/package_files_spec.rb:104:in `block (6 levels) in <main>'
     # ./spec/spec_helper.rb:426:in `block (3 levels) in <top (required)>'
     # ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
     # ./spec/spec_helper.rb:417:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:413:in `block (3 levels) in <top (required)>'
     # ./lib/gitlab/application_context.rb:70:in `with_raw_context'
     # ./spec/spec_helper.rb:413:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:260:in `block (2 levels) in <top (required)>'
     # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
     # ./spec/support/fast_quarantine.rb:22:in `block (2 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'
Finished in 7.53 seconds (files took 38.04 seconds to load)
3 examples, 1 failure
Failed examples:
rspec ./spec/requests/api/package_files_spec.rb:101 # API::PackageFiles GET /projects/:id/packages/:package_id/package_files without the need for a license with pagination params when viewing the first page returns first 2 packages

Note the ids the test expects ([292, 290] - the third and first package file) vs the ids it gets ([290, 291] - the first and second package file).

This happens because the endpoint orders the package files: https://gitlab.com/gitlab-org/gitlab/-/blob/ed916aa622f1ecb2229f88de01d29d63e2aa386d/lib/api/package_files.rb#L39

But the spec does not: https://gitlab.com/gitlab-org/gitlab/-/blob/ed916aa622f1ecb2229f88de01d29d63e2aa386d/spec/requests/api/package_files_spec.rb#L96

Code like packages.package_files issues a query for the package files that does not specify an order:

[8] pry(main)> Packages::Package.first.package_files
  Packages::Package Load (0.5ms)  SELECT "packages_packages".* FROM "packages_packages" ORDER BY "packages_packages"."id" ASC LIMIT 1 /*application:console,db_config_name:main,console_hostname:rick,console_username:jason,line:(pry):8:in `__pry__'*/
  Packages::PackageFile Load (0.3ms)  SELECT "packages_package_files".* FROM "packages_package_files" WHERE "packages_package_files"."package_id" = 1 /*application:console,db_config_name:main,console_hostname:rick,console_username:jason,line:bin/rails:4:in `<main>'*/
=> [#<Packages::PackageFile:0x000000017693cb38
  id: 1,
  package_id: 1,
  created_at: Mon, 02 Oct 2023 20:18:44.089935000 UTC +00:00,
  updated_at: Mon, 02 Oct 2023 20:18:44.089935000 UTC +00:00,
  size: 2048000,
  file_store: 1,
  file_md5: nil,
  file_sha1: nil,
  file_name: "my-module-my-system-0.0.1.tgz",
  file: "my-module-my-system-0.0.1.tgz",
  file_sha256: "ccd8a3066f900bf3bcc5f39d6c708f1106675fb57006594962597dca4bb8edab",
  verification_retry_at: nil,
  verified_at: nil,
  verification_failure: nil,
  verification_retry_count: nil,
  verification_checksum: nil,
  verification_state: 0,
  verification_started_at: nil,
  status: "default",
  new_file_path: nil>]
[9] pry(main)>

So the order in which the package_files is returned is left up to the database, and it may return them in any order.

This MR fixes the issue by adjusting the spec to set the order of the package files in the spec.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Jason Goodman

Merge request reports