Skip to content

Ensure specs run in defined order #319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andyw8
Copy link

@andyw8 andyw8 commented Aug 1, 2025

What this does

By default, RSpec runs the specs in their defined order. But if a developer has --order random set in their environment, such as in the thoughtbot dotfiles here, then ruby_llm's specs will fail. Setting this in the repo's .rspec will take precedence.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Performance improvement

Scope check

  • I read the Contributing Guide
  • This aligns with RubyLLM's focus on LLM communication
  • This isn't application-specific logic that belongs in user code
  • This benefits most users, not just my specific use case

Quality check

  • I ran overcommit --install and all hooks pass
  • I tested my changes thoroughly
  • I updated documentation if needed
  • I didn't modify auto-generated files manually (models.json, aliases.json)

API changes

  • Breaking change
  • New public methods/classes
  • Changed method signatures
  • No API changes

Related issues

@andyw8
Copy link
Author

andyw8 commented Aug 1, 2025

(I left Type of change blank as this doesn't match any of the options)

@tagliala
Copy link

tagliala commented Aug 2, 2025

Hello,

I’m new to this codebase.

Since running specs in random order is a best practice, I suggest investigating why the order affects the specs and enabling config.order = :random in spec_helper.rb instead of forcing the order

I ran a couple of rspec bisect and found (at least):

  • spec/ruby_llm/models_refresh_spec.rb
  • spec/ruby_llm/providers/anthropic/tools_spec.rb

The former is likely modifying some information for the rest of the specs. It should be possible to add an around block to revert the side effects of this method. Something like:

diff --git a/spec/ruby_llm/models_refresh_spec.rb b/spec/ruby_llm/models_refresh_spec.rb
index 28814d3..b89c881 100644
--- a/spec/ruby_llm/models_refresh_spec.rb
+++ b/spec/ruby_llm/models_refresh_spec.rb
@@ -85,6 +85,12 @@ RSpec.describe RubyLLM::Models do
       allow(described_class).to receive_messages(fetch_from_providers: mock_provider_models, fetch_from_parsera: [])
     end
 
+    around do |example|
+      old_instance = described_class.instance_variable_get(:@instance)
+      example.run
+      described_class.instance_variable_set(:@instance, old_instance)
+    end
+
     let(:mock_provider_models) do
       [
         RubyLLM::Model::Info.new(`

The latter appears to be confused by ::Message and ::ToolCall defined in the dummy spec app.

This is a potential fix for the latter:

diff --git a/spec/ruby_llm/providers/anthropic/tools_spec.rb b/spec/ruby_llm/providers/anthropic/tools_spec.rb
index 834decd..a8dbf53 100644
--- a/spec/ruby_llm/providers/anthropic/tools_spec.rb
+++ b/spec/ruby_llm/providers/anthropic/tools_spec.rb
@@ -7,10 +7,10 @@ RSpec.describe RubyLLM::Providers::Anthropic::Tools do
 
   describe '.format_tool_call' do
     let(:msg) do
-      instance_double(Message,
+      instance_double(RubyLLM::Message,
                       content: 'Some content',
                       tool_calls: {
-                        'tool_123' => instance_double(ToolCall,
+                        'tool_123' => instance_double(RubyLLM::ToolCall,
                                                       id: 'tool_123',
                                                       name: 'test_tool',
                                                       arguments: { 'arg1' => 'value1' })
@@ -36,10 +36,10 @@ RSpec.describe RubyLLM::Providers::Anthropic::Tools do
 
     context 'when message has no content' do
       let(:msg) do
-        instance_double(Message,
+        instance_double(RubyLLM::Message,
                         content: nil,
                         tool_calls: {
-                          'tool_123' => instance_double(ToolCall,
+                          'tool_123' => instance_double(RubyLLM::ToolCall,
                                                         id: 'tool_123',
                                                         name: 'test_tool',
                                                         arguments: { 'arg1' => 'value1' })
@@ -65,10 +65,10 @@ RSpec.describe RubyLLM::Providers::Anthropic::Tools do
 
     context 'when message has empty content' do
       let(:msg) do
-        instance_double(Message,
+        instance_double(RubyLLM::Message,
                         content: '',
                         tool_calls: {
-                          'tool_123' => instance_double(ToolCall,
+                          'tool_123' => instance_double(RubyLLM::ToolCall,
                                                         id: 'tool_123',
                                                         name: 'test_tool',
                                                         arguments: { 'arg1' => 'value1' }

@tagliala
Copy link

tagliala commented Aug 2, 2025

Adding a new comment for notification purposes, then I'll edit my previous message:

This should revert the side effects of the first spec

diff --git a/spec/ruby_llm/models_refresh_spec.rb b/spec/ruby_llm/models_refresh_spec.rb
index 28814d3..b89c881 100644
--- a/spec/ruby_llm/models_refresh_spec.rb
+++ b/spec/ruby_llm/models_refresh_spec.rb
@@ -85,6 +85,12 @@ RSpec.describe RubyLLM::Models do
       allow(described_class).to receive_messages(fetch_from_providers: mock_provider_models, fetch_from_parsera: [])
     end
 
+    around do |example|
+      old_instance = described_class.instance_variable_get(:@instance)
+      example.run
+      described_class.instance_variable_set(:@instance, old_instance)
+    end
+
     let(:mock_provider_models) do
       [
         RubyLLM::Model::Info.new(`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants