Run all new backend specs in random order
What does this MR do and why?
With this MR all
All (minus spec/rubocop
- see !93143 (merged)) old specs are still run in defined order and excluded via
spec/support/rspec_order_todo.yml
(10930 files).
Note that the example group description ends with " (random)" if run in random order to help debugging spec failures. See example below.
This MR also enables all RuboCop specs to be run in random order. This has been checked via scripts/rspec_check_order_dependence spec/rubocop
which checks passed spec files and deletes them from spec/support/rspec_order_todo.yml
on success
Refs #337399 (closed).
Mixed mode
Note that running specs in random and defined order still works just fine because randomization happens on example group basis.
For example:
bin/rspec spec/lib/gitlab_spec.rb spec/rubocop/cop_todo_spec.rb
Gitlab
delegates root to GitlabEdition
delegates jh? to GitlabEdition
delegates ee? to GitlabEdition
delegates extensions to GitlabEdition
...
RuboCop::CopTodo (random)
#to_yaml
when previously disabled
...
Gitlab
example group is run in defined order where as RuboCop::CopTodo (random)
(note the (random)
appendix is run in random order.
Reverse order?
When passing RSPEC_ORDER=reverse
you can run specs in reverse order which is used by scripts/rspec_check_order_dependence
.
Reproducibility
In order to reproduce spec failures due to order issues you can use the seed via --seed SEED
. Example: Randomized with seed 27443
.
qa/
?
QA specs are already run in random order and are out of scope of this MR.
Screenshots or screen recordings
scripts/rspec_check_order_dependence spec/rubocop - see !93143 (merged)
|
---|
![]() |
How to set up and validate locally
bin/rspec --format documentation spec/rubocop
Run options: include {:focus=>true}
All examples were filtered out; ignoring {:focus=>true}
Randomized with seed 22503
RuboCop::Cop::Migration::Datetime (random)
when in migration
registers an offense when the ":datetime" data type is used on add_column
registers an offense when the ":timestamp" data type is used on create_table
does not register an offense when the ":datetime_with_timezone" data type is used on create_table
does not register an offense when the ":datetime" data type is not used on add_column
does not register an offense when the ":datetime_with_timezone" data type is used on add_column
registers an offense when the ":timestamp" data type is used on add_column
does not register an offense when the ":datetime" data type is not used on create_table
registers an offense when the ":datetime" data type is used on create_table
when outside of migration
registers no offense
RuboCop::Cop::AvoidReturnFromBlocks (random)
doesn't create more than one offense for nested blocks
doesn't flag violation for return used inside a method definition
doesn't flag violation for next inside a block
doesn't check when block is empty
doesn't flag violation for return inside a lambda
flags violation for return inside a block
flags violation for return inside included > def > block
doesn't flag violation for break inside a block
behaves like examples with def methods
doesn't flag violation for return inside define_method
behaves like examples with def methods
doesn't flag violation for return inside lambda
behaves like examples with whitelisted method
doesn't flag violation for return inside loop
behaves like examples with whitelisted method
doesn't flag violation for return inside times
behaves like examples with whitelisted method
doesn't flag violation for return inside each
behaves like examples with whitelisted method
doesn't flag violation for return inside each_filename
...
Note the (random)
appendix to each example group to denote that it's run in random order.
TODO YAML
Generated via
find spec ee/spec -type f -name "*.rb" | grep -E "_spec\.rb$|frontend/fixtures" | sort | perl -lne "print qq{- './\$_'}" > spec/support/rspec_order_todo.yml
Current TODOs are:
- All spec files ending with
_spec.rb
- All frontend fixtures (matching
frontend/fixtures
)
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
assigned to @splattael
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @rspeicher
,@dzaporozhets
,@marin
,@rymai
,@filipa
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
- A deleted user
added backend documentation labels
1 Warning This merge request is definitely too big (11475 lines changed), please split it into multiple merge requests. 2 Messages CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
doc/development/testing_guide/best_practices.md
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Felipe Artur ( @felipe_artur
) (UTC-3, 5 hours behind@splattael
)Jan Provaznik ( @jprovaznik
) (UTC+2, same timezone as@splattael
)maintenanceworkflow / maintenancepipelines for CI, Danger Aman Luthra ( @aluthra2
) (UTC+5.5, 3.5 hours ahead of@splattael
)Jan Provaznik ( @jprovaznik
) (UTC+2, same timezone as@splattael
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- Resolved by 🤖 GitLab Bot 🤖
@splattael - please add typebug typefeature, typemaintenance or a subtype label to this merge request.- typebug: Defects in shipped code and fixes for those defects. This includes all the bug types (availability, performance, security vulnerability, mobile, etc.)
- typefeature: Effort to deliver new features, feature changes & improvements. This includes all changes as part of new product requirements like application limits.
- typemaintenance: Up-keeping efforts & catch-up corrective improvements that are not Features nor Bugs. This includes restructuring for long-term maintainability, stability, reducing technical debt, improving the contributor experience, or upgrading dependencies.
See the handbook for more guidance on classifying.
This message was created with automation and Engineering Productivity is looking for feedback in this issue:
https://gitlab.com/gitlab-org/quality/engineering-productivity/team/-/issues/43
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
removed backend documentation labels
changed milestone to %15.3
added Technical Writing label
- A deleted user
added backend documentation labels
mentioned in issue #337399 (closed)
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
- Resolved by Rémy Coutable
@gitlab-org/maintainers/rails-backend Thoughts on running all
backend specs in random order?
added sectionops label
added 1 commit
- f3076ee9 - Add a new section "Test order" to Testing Guide's Best Practices
- Resolved by Peter Leitzen
It's worth noting some insights from RSpec creators in https://github.com/rspec/rspec-core/issues/635 where they discussed to make test order
random
the default in RSpec 3. They ended up only enabling it by default withrspec --init
.Edit: Since the creation of GitLab (93efff94) predates the discussion above
config.order = :random
never have been part of GitLab'sspec_helper.rb
. If DZ would have waited only 8 (or so) months to create GitLab we'd have random specs by default alreadyEdited by Peter Leitzen
added 1221 commits
-
f3076ee9...d0459d2e - 1218 commits from branch
master
- d053e904 - Run all new backend specs in random order
- d5e0baca - Add script to check order dependency of RSpec examples
- ceda61f7 - Add a new section "Test order" to Testing Guide's Best Practices
Toggle commit list-
f3076ee9...d0459d2e - 1218 commits from branch
Since you were already involved in discussions, can you do the first review?
requested review from @godfat-gitlab
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
- Resolved by Rémy Coutable
- Resolved by Peter Leitzen
- Resolved by Lin Jen-Shin
- Resolved by Peter Leitzen
- Resolved by Peter Leitzen
removed review request for @godfat-gitlab
added 135 commits
-
ddbd204c...5aba5edb - 132 commits from branch
master
- 4bd3983d - Run all new backend specs in random order
- bda61c69 - Add script to check order dependency of RSpec examples
- f974beb4 - Add a new section "Test order" to Testing Guide's Best Practices
Toggle commit list-
ddbd204c...5aba5edb - 132 commits from branch
requested review from @godfat-gitlab
added 1 commit
- b7c022a3 - Add a new section "Test order" to Testing Guide's Best Practices
removed review request for @godfat-gitlab
requested review from @godfat-gitlab
added 995 commits
-
7fdad77d...169e9b61 - 992 commits from branch
master
- 8ccf8639 - Run all new backend specs in random order
- f0758e72 - Add script to check order dependency of RSpec examples
- 217e56d5 - Add a new section "Test order" to Testing Guide's Best Practices
Toggle commit list-
7fdad77d...169e9b61 - 992 commits from branch
- Resolved by Rémy Coutable
@godfat-gitlab
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.
For more info, please refer to the following links:
- Resolved by Rémy Coutable
@splattael Thank you, looks good to me!
@rymai Could you please review as the maintainer? Thanks!
requested review from @rymai and removed review request for @godfat-gitlab
- Resolved by Rémy Coutable
- Resolved by Rémy Coutable
removed review request for @rymai
requested review from @rymai
added 583 commits
-
54d07556...3d24bfa7 - 580 commits from branch
master
- b48cbf81 - Run all new backend specs in random order
- 82bd8a97 - Add script to check order dependency of RSpec examples
- 93cee6dd - Add a new section "Test order" to Testing Guide's Best Practices
Toggle commit list-
54d07556...3d24bfa7 - 580 commits from branch
added 594 commits
-
93cee6dd...d25f16b3 - 591 commits from branch
master
- 83885682 - Run all new backend specs in random order
- 0d3008cb - Add script to check order dependency of RSpec examples
- c29334ec - Add a new section "Test order" to Testing Guide's Best Practices
Toggle commit list-
93cee6dd...d25f16b3 - 591 commits from branch
- Resolved by Rémy Coutable
added maintenanceusability label
mentioned in issue #350700
changed milestone to %15.4
added missed:15.3 label
added 744 commits
-
c29334ec...4fa1f8e2 - 741 commits from branch
master
- dfceae5d - Run all new backend specs in random order
- f7b86604 - Add script to check order dependency of RSpec examples
- c92c9ef6 - Add a new section "Test order" to Testing Guide's Best Practices
Toggle commit list-
c29334ec...4fa1f8e2 - 741 commits from branch
- Resolved by Peter Leitzen
@splattael Looks good to me, thanks!
1 Warning This merge request is definitely too big (11475 lines changed), please split it into multiple merge requests. 2 Messages CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
doc/development/testing_guide/best_practices.md
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Felipe Artur ( @felipe_artur
) (UTC-3, 5 hours behind@splattael
)Jan Provaznik ( @jprovaznik
) (UTC+2, same timezone as@splattael
)maintenanceworkflow / maintenancepipelines for CI, Danger Aman Luthra ( @aluthra2
) (UTC+5.5, 3.5 hours ahead of@splattael
)Jan Provaznik ( @jprovaznik
) (UTC+2, same timezone as@splattael
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangerenabled an automatic merge when the pipeline for f261161b succeeds
mentioned in commit bf8ef431
mentioned in merge request !93143 (merged)
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
Announced in
#backend
,#backend_maintainers
,#development
and Engineering: Week in Review.added workflowproduction label and removed workflowstaging label
mentioned in merge request !95640 (merged)
mentioned in merge request !96137 (merged)
- spec/support/rspec_order.rb 0 → 100644
33 end 34 end 35 36 RSpec.configure do |config| 37 # Useful to find order-dependent specs. 38 config.register_ordering(:reverse, &:reverse) 39 40 # Randomization can be reproduced across test runs. 41 Kernel.srand config.seed 42 43 config.on_example_group_definition do |example_group| 44 order = Support::RspecOrder.order_for(example_group) 45 46 if order 47 example_group.metadata[:order] = order.to_sym 48 example_group.metadata[:description] += " (order #{order})" Appending
(order random)
fails in cases where RSpec is using the description to render HAML templates in HAML specs!96137 (merged) is removing this appendix again and we are looking to find a different solution.
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
added releasedcandidate label
mentioned in issue #407877
#407877 which tracks that all backend specs should be run in random order.
mentioned in merge request !124300 (merged)
mentioned in merge request !157448 (merged)