The following discussion from !205021 should be addressed: - [ ] @cwoolley-gitlab started a [discussion](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/205021#note_2758883126): (+4 comments) > **issue:** We should always use `let_it_be` when possible (https://docs.gitlab.com/development/testing_guide/best_practices/#lets-talk-about-let), unless there's a reason not to (usually due to complex `before` setup which can interact with `let_it_be`, which is implmented with `before` under the covers). > > But I didn't see why it should be necessary here. > > I dug into it for a while, and realized there was an existing bug in this test, because the fixture was getting created with workspace_variable records already associated. > > So, the reason your test was failing was because you were finding the wrong variable record with a nil value (from the original variable creation). > > Below is a patch to fix that. > > **issue** However, this uncovered another bug in this design. Even with the factory, the default value for the `gl_agentw_token` variable should never be `nil`, it should be empty string. > > This is because the factory calls `ee/lib/remote_development/workspace_operations/create/workspace_variables_builder.rb` directly, but you have added the empty-string-defaulting logic to `ee/lib/remote_development/workspace_operations/create/workspace_variables_creator.rb` instead of `ee/lib/remote_development/workspace_operations/create/workspace_variables_builder.rb` > > So, I think the logic for the defaulting needs to be moved to `ee/lib/remote_development/workspace_operations/create/workspace_variables_builder.rb` instead. > > So, the patch below may only be partially relevant - we should still fix the fixture, but the actual example and test should probably move to the spec for `ee/lib/remote_development/workspace_operations/create/workspace_variables_builder.rb` instead. > > <details> > <summary>patch</summary> > > > ```diff > Index: ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_creator_spec.rb > IDEA additional info: > Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP > <+>UTF-8 > =================================================================== > diff --git a/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_creator_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_creator_spec.rb > --- a/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_creator_spec.rb (revision b3982bd905d34aeb4dd6806a8b27ffd555c318c4) > +++ b/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_creator_spec.rb (date 1758156868863) > @@ -12,8 +12,11 @@ > let_it_be(:user) { create(:user) } > let_it_be(:personal_access_token) { create(:personal_access_token, user: user) } > # The desired state of the workspace is set to running so that a workspace token gets associated to it. > - let(:workspace) do > - create(:workspace, user: user, personal_access_token: personal_access_token, desired_state: states_module::RUNNING) > + let_it_be(:workspace, refind: true) do > + create( > + :workspace, :without_workspace_variables, > + user: user, personal_access_token: personal_access_token, desired_state: states_module::RUNNING > + ) > end > > let(:vscode_extension_marketplace) do > @@ -53,6 +56,12 @@ > described_class.create(context) # rubocop:disable Rails/SaveBang -- this is not an ActiveRecord method > end > > + it "has fixture sanity check" do > + # We must ensure the workspace fixture, which simulates a workspace in the ROP create chain in the process of being > + # created, does not contain any pre-existing associated workspace_variable records > + expect(workspace.workspace_variables.count).to eq(0) > + end > + > context "when workspace variables create is successful" do > let(:valid_variable_type) { RemoteDevelopment::Enums::WorkspaceVariable::ENVIRONMENT_TYPE } > let(:variable_type) { valid_variable_type } > @@ -61,8 +70,8 @@ > it "creates the workspace variable records and returns ok result containing original context" do > expect { result }.to change { workspace.workspace_variables.count }.by(expected_number_of_records_saved) > > - expect(RemoteDevelopment::WorkspaceVariable.find_by_key("key1").value).to eq("value 1") > - expect(RemoteDevelopment::WorkspaceVariable.find_by_key("key2").value).to eq("value 2") > + expect(workspace.workspace_variables.find_by_key("key1").value).to eq("value 1") > + expect(workspace.workspace_variables.find_by_key("key2").value).to eq("value 2") > > expect(result).to be_ok_result(context) > end > @@ -84,7 +93,7 @@ > it "creates the workspace variable records and returns ok result containing original context" do > expect { result }.to change { workspace.workspace_variables.count }.by(expected_number_of_records_saved) > > - expect(RemoteDevelopment::WorkspaceVariable.find_by_key(agentw_token_file_name).value).not_to be_empty > + expect(workspace.workspace_variables.find_by_key(agentw_token_file_name).value).to match(/glwt-.*/) > > expect(result).to be_ok_result(context) > end > ``` > > > </details>