From 754f6f4c4bd00c61701c5d1a1c8a553f22e53d8a Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Sun, 30 Jul 2023 14:52:54 -0700 Subject: [PATCH] Introduce Remote Dev fast_spec_helper - Also includes other minor refactor/cleanup --- ee/lib/remote_development/README.md | 9 ++++++ .../remote_development/agent_config/main.rb | 1 - .../workspaces/create/main.rb | 1 - .../workspaces/update/main.rb | 1 - .../agent_config/main_spec.rb | 14 ++++----- .../remote_development/fast_spec_helper.rb | 31 +++++++++++++++++++ .../workspaces/create/main_spec.rb | 29 ++++++++--------- .../workspaces/update/main_spec.rb | 12 +++---- .../railway_oriented_programming_helpers.rb | 10 +++--- 9 files changed, 73 insertions(+), 35 deletions(-) create mode 100644 ee/spec/lib/remote_development/fast_spec_helper.rb diff --git a/ee/lib/remote_development/README.md b/ee/lib/remote_development/README.md index a0e5f0c4d33365..7619bcbec06935 100644 --- a/ee/lib/remote_development/README.md +++ b/ee/lib/remote_development/README.md @@ -18,6 +18,7 @@ - All the domain logic lives under `ee/lib/remote_development`. Unless you are changing the DB schema or API structure, your changes will probably be made here. - The `Main` class is the entry point for each sub-module, and is found at `ee/lib/remote_development/**/main.rb` - Have a look through the ["Railway Oriented Programming"](https://fsharpforfunandprofit.com/rop/) presentation slides (middle of that page) to understand the patterns used in the Domain Logic layer. +- Prefer `ee/spec/lib/remote_development/fast_spec_helper.rb` instead of `spec_helper` where possible. See [Avoid coupling Domain Logic layer to Rails application](#avoid-coupling-domain-logic-layer-to-rails-application). - Use `scripts/remote_development/run-smoke-test-suite.sh` locally, to get a faster feedback than pushing to CI and waiting for a build. - Use `scripts/remote_development/run-e2e-tests.sh` to easily run the QA E2E tests. - If you use [RubyMine](https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/rubymine/), you will get a lot of extra help, because we try to keep the `Inspect Code` clean and green for all Remote Development files, and also maintain YARD annotations, which means you will get fast in-IDE feedback about many errors such as type violations, etc, which are not caught by the standard Gitlab static linters such as RuboCop, ESLint, etc. @@ -53,6 +54,14 @@ An example of this is how we avoid coupling the Domain Logic layer to the Servic This overall approach is aligned with [our direction towards a more modular monolith](https://docs.gitlab.com/ee/architecture/blueprints/modular_monolith/). See that document for more information on the motivations and patterns. Specifically, see the `References` sub-page and reference to the the [`hexagonal architecture ports and adapters`](https://www.google.com/search?q=hexagonal+architecture+ports+and+adapters&tbm=isch) pattern, which includes [this article with an example of this architecture](https://herbertograca.com/2017/11/16/explicit-architecture-01-ddd-hexagonal-onion-clean-cqrs-how-i-put-it-all-together/) +### Avoid coupling Domain Logic layer to Rails application + +This layered approach also implies that we avoid directly referring to classes which are part of the Rails application from within the Domain Logic layer. + +If possible, we prefer to inject instances of these classes, or the classes themselves, into the Domain Logic layer from the Service layer. + +This also means that we can use `ee/spec/lib/remote_development/fast_spec_helper.rb` in most places in the Domain Logic layer. However, we may use the normal `spec_helper` for Domain Logic classes which make direct use of Rails. For example, classes which directly use ActiveRecord models and/or associations, where the usage of `fast_spec_helper` would require significant mocking, and not provide as much coverage of the ActiveRecord interactions. + ## Type safety The Remote Development domain leverages type safety where possible and pragmatic. This allows us to have some run-time safety nets in addition to test coverage, and also helps RubyMine provide useful warnings when the wrong types are used. diff --git a/ee/lib/remote_development/agent_config/main.rb b/ee/lib/remote_development/agent_config/main.rb index 012fcf26a4e0db..e4f61606de7bc2 100644 --- a/ee/lib/remote_development/agent_config/main.rb +++ b/ee/lib/remote_development/agent_config/main.rb @@ -32,7 +32,6 @@ def self.main(value) message.context => { skipped_reason: Symbol } # Type-check the payload before returning it { status: :success, payload: message.context } in { ok: AgentConfigUpdateSuccessful => message } - message.context => { remote_development_agent_config: RemoteDevelopmentAgentConfig } # Type-check the payload { status: :success, payload: message.context } else raise UnmatchedResultError.new(result: result) diff --git a/ee/lib/remote_development/workspaces/create/main.rb b/ee/lib/remote_development/workspaces/create/main.rb index 853c06d4bdb582..71afb6f908c6b4 100644 --- a/ee/lib/remote_development/workspaces/create/main.rb +++ b/ee/lib/remote_development/workspaces/create/main.rb @@ -45,7 +45,6 @@ def self.main(value) in { err: WorkspaceCreateFailed => message } generate_error_response_from_message(message: message, reason: :bad_request) in { ok: WorkspaceCreateSuccessful => message } - message.context => { workspace: Workspace } # Type-check the payload before returning it { status: :success, payload: message.context } else raise UnmatchedResultError.new(result: result) diff --git a/ee/lib/remote_development/workspaces/update/main.rb b/ee/lib/remote_development/workspaces/update/main.rb index 1c170b46e79f34..67ace986a9678d 100644 --- a/ee/lib/remote_development/workspaces/update/main.rb +++ b/ee/lib/remote_development/workspaces/update/main.rb @@ -25,7 +25,6 @@ def self.main(value) in { err: WorkspaceUpdateFailed => message } generate_error_response_from_message(message: message, reason: :bad_request) in { ok: WorkspaceUpdateSuccessful => message } - message.context => { workspace: Workspace } # Type-check the payload before returning it { status: :success, payload: message.context } else raise UnmatchedResultError.new(result: result) diff --git a/ee/spec/lib/remote_development/agent_config/main_spec.rb b/ee/spec/lib/remote_development/agent_config/main_spec.rb index 32a0cb1c208dbe..8fc4cca0867b54 100644 --- a/ee/spec/lib/remote_development/agent_config/main_spec.rb +++ b/ee/spec/lib/remote_development/agent_config/main_spec.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true -require 'spec_helper' +require_relative '../fast_spec_helper' RSpec.describe RemoteDevelopment::AgentConfig::Main, feature_category: :remote_development do include RemoteDevelopment::RailwayOrientedProgrammingHelpers - let(:initial_value) { {} } + let(:value) { {} } let(:error_details) { 'some error details' } let(:err_message_context) { { details: error_details } } @@ -21,7 +21,7 @@ # Subject - subject(:response) { described_class.main(initial_value) } + subject(:response) { described_class.main(value) } before do allow(license_checker_class).to receive(:method) { license_checker_method } @@ -78,7 +78,7 @@ stub_methods_to_return_ok_result( license_checker_method ) - allow(updater_method).to receive(:call).with(initial_value) do + allow(updater_method).to receive(:call).with(value) do Result.ok( RemoteDevelopment::Messages::AgentConfigUpdateSkippedBecauseNoConfigFileEntryFound.new(skip_updater_context) ) @@ -94,13 +94,13 @@ end context 'when the Updater returns an AgentConfigUpdateSuccessful Result' do - let(:agent_config) { build_stubbed(:remote_development_agent_config) } + let(:agent_config) { instance_double("RemoteDevelopment::RemoteDevelopmentAgentConfig") } before do stub_methods_to_return_ok_result( license_checker_method ) - allow(updater_method).to receive(:call).with(initial_value) do + allow(updater_method).to receive(:call).with(value) do Result.ok(RemoteDevelopment::Messages::AgentConfigUpdateSuccessful.new( { remote_development_agent_config: agent_config } )) @@ -123,7 +123,7 @@ stub_methods_to_return_ok_result( license_checker_method ) - allow(updater_method).to receive(:call).with(initial_value) do + allow(updater_method).to receive(:call).with(value) do Result.err(RemoteDevelopment::Messages::AgentConfigUpdateSuccessful.new) end end diff --git a/ee/spec/lib/remote_development/fast_spec_helper.rb b/ee/spec/lib/remote_development/fast_spec_helper.rb new file mode 100644 index 00000000000000..24987e1cdca8f8 --- /dev/null +++ b/ee/spec/lib/remote_development/fast_spec_helper.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +if $LOADED_FEATURES.include?(File.expand_path('../../../../spec/spec_helper.rb', __dir__.to_s)) + # return if spec_helper is already loaded, so we don't accidentally override any configuration in it + return +end + +if ENV['SPRING_TMP_PATH'] + warn "\n\n\nERROR: Spring is detected as running due to ENV['SPRING_TMP_PATH']=#{ENV['SPRING_TMP_PATH']} found.\n\n" \ + "Do not run #{__FILE__} with Spring enabled, it can cause Zeitwerk errors.\n\n" \ + "Exiting.\n\n" + + exit 1 +end + +require_relative '../../../../spec/fast_spec_helper' +require_relative '../../../../spec/support/helpers/next_instance_of' +require_relative '../../../../spec/support/matchers/result_matchers' +require_relative '../../support/helpers/remote_development/railway_oriented_programming_helpers' +require_relative '../../support/shared_contexts/remote_development/agent_info_status_fixture_not_implemented_error' +require_relative '../../support/shared_contexts/remote_development/remote_development_shared_contexts' + +require 'rspec-parameterized' +require 'json_schemer' + +RSpec.configure do |config| + config.include NextInstanceOf + config.mock_with :rspec do |mocks| + mocks.verify_doubled_constant_names = false + end +end diff --git a/ee/spec/lib/remote_development/workspaces/create/main_spec.rb b/ee/spec/lib/remote_development/workspaces/create/main_spec.rb index 992d990c2c25c9..4a9a1219941750 100644 --- a/ee/spec/lib/remote_development/workspaces/create/main_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/main_spec.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true -require 'spec_helper' +require_relative '../../fast_spec_helper' RSpec.describe RemoteDevelopment::Workspaces::Create::Main, feature_category: :remote_development do include RemoteDevelopment::RailwayOrientedProgrammingHelpers - let(:initial_value) { {} } + let(:value) { {} } let(:error_details) { 'some error details' } let(:err_message_context) { { details: error_details } } @@ -39,7 +39,7 @@ # Subject - subject(:response) { described_class.main(initial_value) } + subject(:response) { described_class.main(value) } before do allow(authorizer_class).to receive(:method) { authorizer_method } @@ -55,10 +55,13 @@ end context 'when the Authorizer returns an err Result' do - it 'returns an unauthorized error response' do - expect(authorizer_method).to receive(:call).with(initial_value) do + before do + allow(authorizer_method).to receive(:call).with(value) do Result.err(RemoteDevelopment::Messages::Unauthorized.new) end + end + + it 'returns an unauthorized error response' do expect(response).to eq({ status: :error, message: 'Unauthorized', reason: :unauthorized }) end end @@ -189,7 +192,7 @@ end context 'when the Creator returns an ok Result' do - let(:workspace) { build_stubbed(:workspace) } + let(:workspace) { instance_double("RemoteDevelopment::Workspace") } before do stub_methods_to_return_ok_result( @@ -206,13 +209,12 @@ editor_component_injector_method, project_cloner_component_injector_method ) - end - - it 'returns a workspace create success response with the workspace as the payload' do - expect(creator_method).to receive(:call).with(initial_value) do + allow(creator_method).to receive(:call).with(value) do Result.ok(RemoteDevelopment::Messages::WorkspaceCreateSuccessful.new({ workspace: workspace })) end + end + it 'returns a workspace create success response with the workspace as the payload' do expect(response).to eq({ status: :success, payload: { workspace: workspace } @@ -238,13 +240,12 @@ editor_component_injector_method, project_cloner_component_injector_method ) - end - - it 'returns a workspace create failed error response' do - expect(creator_method).to receive(:call).with(initial_value) do + allow(creator_method).to receive(:call).with(value) do Result.err(RemoteDevelopment::Messages::WorkspaceCreateSuccessful.new) end + end + it 'raises an UnmatchedResultError' do expect { response }.to raise_error(RemoteDevelopment::UnmatchedResultError) end end diff --git a/ee/spec/lib/remote_development/workspaces/update/main_spec.rb b/ee/spec/lib/remote_development/workspaces/update/main_spec.rb index bbcdff1fddd085..2c54b9de928f03 100644 --- a/ee/spec/lib/remote_development/workspaces/update/main_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/update/main_spec.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true -require 'spec_helper' +require_relative '../../fast_spec_helper' RSpec.describe RemoteDevelopment::Workspaces::Update::Main, feature_category: :remote_development do include RemoteDevelopment::RailwayOrientedProgrammingHelpers - let(:initial_value) { {} } + let(:value) { {} } let(:error_details) { 'some error details' } let(:err_message_context) { { details: error_details } } @@ -21,7 +21,7 @@ # Subject - subject(:response) { described_class.main(initial_value) } + subject(:response) { described_class.main(value) } before do allow(authorizer_class).to receive(:method) { authorizer_method } @@ -70,13 +70,13 @@ end context 'when the Updater returns an ok Result' do - let(:workspace) { build_stubbed(:workspace) } + let(:workspace) { instance_double("RemoteDevelopment::Workspace") } before do stub_methods_to_return_ok_result( authorizer_method ) - allow(updater_method).to receive(:call).with(initial_value) do + allow(updater_method).to receive(:call).with(value) do Result.ok(RemoteDevelopment::Messages::WorkspaceUpdateSuccessful.new({ workspace: workspace })) end end @@ -96,7 +96,7 @@ stub_methods_to_return_ok_result( authorizer_method ) - allow(updater_method).to receive(:call).with(initial_value) do + allow(updater_method).to receive(:call).with(value) do # Note that this is not pattern matched, because there's no match for a `Result.err` with this message. Result.err(RemoteDevelopment::Messages::WorkspaceUpdateSuccessful.new) end diff --git a/ee/spec/support/helpers/remote_development/railway_oriented_programming_helpers.rb b/ee/spec/support/helpers/remote_development/railway_oriented_programming_helpers.rb index 80d66cdb8027a6..a1c0d5aecf3edc 100644 --- a/ee/spec/support/helpers/remote_development/railway_oriented_programming_helpers.rb +++ b/ee/spec/support/helpers/remote_development/railway_oriented_programming_helpers.rb @@ -4,16 +4,16 @@ module RemoteDevelopment # noinspection RubyClassModuleNamingConvention, RubyInstanceMethodNamingConvention - See https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/code-inspection/why-are-there-noinspection-comments/ # noinspection RubyInstanceMethodNamingConvention - See https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/code-inspection/why-are-there-noinspection-comments/ module RailwayOrientedProgrammingHelpers - # NOTE: Depends upon `initial_value` being defined in the including spec + # NOTE: Depends upon `value` being defined in the including spec def stub_methods_to_return_ok_result(*methods) methods.each do |method| - allow(method).to receive(:call).with(initial_value) { Result.ok(initial_value) } + allow(method).to receive(:call).with(value) { Result.ok(value) } end end - # NOTE: Depends upon `initial_value` and `err_message_context` being defined in the including spec + # NOTE: Depends upon `value` and `err_message_context` being defined in the including spec def stub_methods_to_return_err_result(method:, message_class:) - allow(method).to receive(:call).with(initial_value) do + allow(method).to receive(:call).with(value) do # noinspection RubyResolve Result.err(message_class.new(err_message_context)) end @@ -21,7 +21,7 @@ def stub_methods_to_return_err_result(method:, message_class:) def stub_methods_to_return_value(*methods) methods.each do |method| - allow(method).to receive(:call).with(initial_value) { initial_value } + allow(method).to receive(:call).with(value) { value } end end end -- GitLab