Skip to content

Speed up projects merge requests controller specs

Peter Leitzen requested to merge pl-spec-controller-projects-mr-perf into master

What does this MR do?

This MR improves the performance of projects merge requests controller specs by using let_it_be. It also fixes some 👮 offenses for Rails/SaveBang.

Related to &3752.

Contributes to https://gitlab.com/gitlab-org/plan/-/issues/145 and #220040 (closed).

Here's the output from test-prof when run with FPROF=1. Note:

  • Total amount of factories created went down from 827 to 444 👍
  • Total events went down from 32846 to 21749
  • Queries saved: 11097

Before

[TEST PROF INFO] EventProf results for sql.active_record

Total time: 00:43.357 of 02:06.514 (34.27%)
Total events: 32846

Top 5 slowest suites (by time):

Projects::Mer...estsController (./spec/controllers/projects/merge_requests_controller_spec.rb:5) – 00:43.357 (32846 / 172) of 02:06.514 (34.27%)


Finished in 2 minutes 13.2 seconds (files took 1.66 seconds to load)
173 examples, 0 failure

[TEST PROF INFO] Factories usage

 Total: 827
 Total top-level: 581
 Total time: 80.0098s
 Total uniq factories: 20

   total   top-level     total time      time per call      top-level time               name

     193         193       36.3331s            0.1883s            36.3331s            project
     175           4        8.3180s            0.0475s             0.1247s          namespace
     105         105       16.4824s            0.1570s            16.4824s      merge_request
      88          88       17.0537s            0.1938s            17.0537s merge_request_with_diffs
      86          49        2.9574s            0.0344s             1.8930s        ci_pipeline
      35          35        1.6236s            0.0464s             1.6236s               user
      22           6        0.8162s            0.0371s             0.1130s    ci_job_artifact
      18          18        0.3855s            0.0214s             0.3855s discussion_note_on_merge_request
      18          18        1.0362s            0.0576s             1.0362s              group
      18           0        0.0963s            0.0054s             0.0000s namespace_settings
      15          15        0.9250s            0.0617s             0.9250s           ci_build
      13          13        0.2572s            0.0198s             0.2572s        environment
      13          13        1.3190s            0.1015s             1.3190s         deployment
       8           8        0.4026s            0.0503s             0.4026s              issue
       6           6        0.1945s            0.0324s             0.1945s  ci_empty_pipeline
       6           2        0.4602s            0.0767s             0.1320s diff_note_on_merge_request
       4           4        1.3790s            0.3448s             1.3790s merge_request_with_diff_notes
       2           2        0.2505s            0.1253s             0.2505s invalid_merge_request
       1           1        0.0078s            0.0078s             0.0078s            license
       1           1        0.0970s            0.0970s             0.0970s diff_note_on_commit

After

[TEST PROF INFO] EventProf results for sql.active_record

Total time: 00:27.435 of 01:25.549 (32.07%)
Total events: 21749

Top 5 slowest suites (by time):

Projects::Mer...estsController (./spec/controllers/projects/merge_requests_controller_spec.rb:5) – 00:27.435 (21749 / 172) of 01:25.549 (32.07%)


Finished in 1 minute 32.56 seconds (files took 1.68 seconds to load)
173 examples, 0 failure

[TEST PROF INFO] Factories usage

 Total: 444
 Total top-level: 385
 Total time: 38.3504s
 Total uniq factories: 20

   total   top-level     total time      time per call      top-level time               name

      88          88       14.4287s            0.1640s            14.4287s merge_request_with_diffs
      73          73        9.8153s            0.1345s             9.8153s      merge_request
      54          49        2.8688s            0.0531s             2.7493s        ci_pipeline
      35          35        5.0335s            0.1438s             5.0335s            project
      32          32        1.4473s            0.0452s             1.4473s               user
      22           6        1.6126s            0.0733s             0.0901s    ci_job_artifact
      18          18        0.3901s            0.0217s             0.3901s discussion_note_on_merge_request
      18          18        0.4898s            0.0272s             0.4898s              group
      18           0        0.0766s            0.0043s             0.0000s namespace_settings
      17           1        0.7966s            0.0469s             0.0347s          namespace
      15          15        0.6921s            0.0461s             0.6921s           ci_build
      13          13        0.1999s            0.0154s             0.1999s        environment
      13          13        1.0962s            0.0843s             1.0962s         deployment
       8           8        0.3445s            0.0431s             0.3445s              issue
       6           6        0.2295s            0.0382s             0.2295s  ci_empty_pipeline
       6           2        0.4106s            0.0684s             0.1327s diff_note_on_merge_request
       4           4        0.8722s            0.2180s             0.8722s merge_request_with_diff_notes
       2           2        0.2057s            0.1028s             0.2057s invalid_merge_request
       1           1        0.0095s            0.0095s             0.0095s            license
       1           1        0.0893s            0.0893s             0.0893s diff_note_on_commit

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Peter Leitzen

Merge request reports