Skip to content

Draft: Introduce bare_project factory with minimal associations

Igor Drozdov requested to merge id-introduce-bare-project into master

What does this MR do?

CI pipelines factory has project as an association. When we run FPROF=1 rspec spec/models/ci/pipeline_spec.rb, we see the following numbers:

total   top-level     total time      time per call      top-level time               name
208          76       35.4191s            0.1703s            13.4195s            project

which means that 208 - 76 = 132 projects have been created implicitly via cascading. Project creation isn't a cheap operation and in order to optimize the time, we can either:

  1. Propagate a project, which has been created via let_it_be for a suite, to all the pipelines (requires many changes and works only for the cases when the project isn't mutated)
  2. Use https://github.com/test-prof/test-prof/blob/master/docs/recipes/any_fixture.md (doesn't require many changes, but also works for immutable projects and introduces implicit global state)
  3. Optimize the time spent on project creation (works for mutable projects, has global optimization impact)

This merge request follows the third option. Most of the tests don't need all the instances created by project model callbacks. We can skip them in a separate factory and make this factory the default. The disadvantage of this approach is that most of the project associations aren't created within the factory and future developers may encounter something like project.statistics is nil error. I wonder how inconvenient it can be 🤔 I've added a comment to the factories' files, assuming it's the first place to go when a developer sees such errors.

Stats for spec/models/ci/pipeline_spec.rb

Before

  • Total events: 31159
  • Finished in 1 minute 59.47 seconds

After

  • Total events: 25314
  • Finished in 1 minute 28.1 seconds

After running all specs changed in this merge request:

FPROF=1 rspec ee/spec/lib/gitlab/auth_spec.rb ee/spec/workers/build_finished_worker_spec.rb spec/controllers/admin/runners_controller_spec.rb spec/controllers/projects/badges_controller_spec.rb spec/features/admin/admin_builds_spec.rb spec/lib/gitlab/auth_spec.rb spec/models/ci/job_artifact_spec.rb spec/models/concerns/batch_destroy_dependent_associations_spec.rb spec/presenters/ci/build_runner_presenter_spec.rb spec/requests/jwt_controller_spec.rb spec/serializers/build_details_entity_spec.rb spec/serializers/pipeline_details_entity_spec.rb spec/serializers/pipeline_serializer_spec.rb spec/services/ci/play_manual_stage_service_spec.rb spec/models/ci/pipeline_spec.rb

Before

Finished in 5 minutes 42 seconds

[TEST PROF INFO] Factories usage
   total   top-level     total time      time per call      top-level time               name
     553         112      127.1291s            0.2299s            25.1823s            project

After

Finished in 4 minutes 5.5 seconds

[TEST PROF INFO] Factories usage
   total   top-level     total time      time per call      top-level time               name
     374          19       23.1489s            0.0619s             1.5380s       bare_project
     159         125       33.4440s            0.2103s            25.3336s            project

The total runtime doesn't have a drastic drop, but I'm still curious about the global impact. The interesting part is time per call: 0.23s vs 0.062s is a tangible difference and, if we continue with this approach, can help to optimize the time spent on project creation for other factories as well.

Edited by Igor Drozdov

Merge request reports