Follow-up from "Move Feature Flag Services to Core RUN AS-IF-FOSS"
The following discussions from !42645 (merged) should be addressed:
-
@jagood started a discussion: (+2 comments) Would you please review this, @atroschinetz? Thanks!
-
@splattael started a discussion: Non-blocking How about using
let_it_be
+before_all
to🚀 specs helping https://gitlab.com/gitlab-org/plan/-/issues/145?🙏 -
@splattael started a discussion: Non-blocking
let_it_be
would help here as well💪 -
@splattael started a discussion: Non-blocking To demonstrate what
let_it_be
+before_all
can do for you:[TEST PROF INFO] EventProf results for sql.active_record Total time: 00:05.549 of 00:10.376 (53.48%) Total events: 4068 Top 5 slowest suites (by time): FeatureFlags::UpdateService (./spec/services/feature_flags/update_service_spec.rb:5) – 00:05.549 (4068 / 22) of 00:10.376 (53.48%) Finished in 17.45 seconds (files took 2.39 seconds to load) 22 examples, 0 failures [TEST PROF INFO] Factories usage Total: 112 Total top-level: 90 Total time: 6.9119s Total uniq factories: 6 total top-level total time time per call top-level time name 44 44 1.5898s 0.0361s 1.5898s user 22 22 4.8458s 0.2203s 4.8458s project 22 0 1.3790s 0.0627s 0.0000s namespace 22 22 0.4638s 0.0211s 0.4638s operations_feature_flag 1 1 0.0072s 0.0072s 0.0072s license 1 1 0.0053s 0.0053s 0.0053s operations_feature_flag_scope
[TEST PROF INFO] EventProf results for sql.active_record Total time: 00:00.856 of 00:02.047 (41.86%) Total events: 665 Top 5 slowest suites (by time): FeatureFlags::UpdateService (./spec/services/feature_flags/update_service_spec.rb:5) – 00:00.856 (665 / 22) of 00:02.047 (41.86%) Finished in 9.25 seconds (files took 2.22 seconds to load) 22 examples, 0 failures [TEST PROF INFO] Factories usage Total: 7 Total top-level: 6 Total time: 0.6520s Total uniq factories: 6 total top-level total time time per call top-level time name 2 2 0.0807s 0.0403s 0.0807s user 1 1 0.0069s 0.0069s 0.0069s license 1 1 0.5032s 0.5032s 0.5032s project 1 0 0.0823s 0.0823s 0.0000s namespace 1 1 0.0529s 0.0529s 0.0529s operations_feature_flag 1 1 0.0084s 0.0084s 0.0084s operations_feature_flag_scope
We are from 4068 down to 665 total events
❤ just with this one weird trick:The diff
diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index 16c3ff23443..766687807f0 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -3,13 +3,13 @@ require 'spec_helper' RSpec.describe FeatureFlags::UpdateService do - let(:project) { create(:project) } - let(:developer) { create(:user) } - let(:reporter) { create(:user) } - let(:user) { developer } - let(:feature_flag) { create(:operations_feature_flag, project: project, active: true) } + let_it_be(:project) { create(:project) } + let_it_be(:developer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:user) { developer } + let_it_be_with_reload(:feature_flag) { create(:operations_feature_flag, project: project, active: true) } - before do + before_all do project.add_developer(developer) project.add_reporter(reporter) end
Again, it's all non-blocking
Three more suggestions:
- Should we externalize (I18n) error messages in service?
- Should we move the environment scope check comparing with
"*"
into a utility method since"*"
is used several times: - Should we move the
new_verison
check into a utility method as well?