Skip to content

Conversation

@swaathi
Copy link
Contributor

@swaathi swaathi commented Aug 5, 2025

A specific bug occurs with TodoList specs where the proc that is captured by the exception refers to a portion of the code which returns an array but outputs a string.

Screenshot 2025-08-05 at 11 17 29 PM

This array causes issues when showing the HTML diff:

Screenshot 2025-08-05 at 10 45 55 PM

To solve this, I capture the output of the proc instead of calling it.

Testing

Test this using the following script (add it to the root of this repository):

# bundle exec rspec stdout_demo.rb --format RSpec::EnrichedJson::Formatters::EnrichedJsonFormatter -r ./lib/rspec/enriched_json

RSpec.describe "stdout" do
  it "captures actual when spec fails" do
    expect { puts "Hello" }.to output("Bye\n").to_stdout
  end

  it "captures actual when spec passes" do
    expect { puts "Hello" }.to output("Hello\n").to_stdout
  end
end

Both cases should return an actual that is a string — not an array.

Learn Output

The specs on Learn now look like this:

When the spec fails:
Screenshot 2025-08-05 at 11 30 51 PM

When the spec passes:
Screenshot 2025-08-05 at 11 31 27 PM

Important

Capture Proc output to stdout in serialize_value and add tests for output matchers.

  • Behavior:
    • Modify serialize_value in ExpectationHelperWrapper to capture Proc output to stdout instead of calling it directly.
    • Add "Output Matchers" category in demo.rb.
  • Tests:
    • Remove Proc serialization test in oj_serialization_spec.rb.
    • Add stdout_matcher_spec.rb to test capturing of stdout output for passing and failing cases.
  • Version:
    • Increment version to 0.8.3 in version.rb.

This description was created by Ellipsis for 28c7483. You can customize this summary. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to 30b3388 in 1 minute and 41 seconds. Click for details.
  • Reviewed 117 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/rspec/enriched_json/expectation_helper_wrapper.rb:50
  • Draft comment:
    Using global $stdout reassignment may lead to issues in concurrent environments. Consider a thread-safe approach if needed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a legitimate concern - modifying global state like $stdout can cause issues if multiple threads try to capture output simultaneously. However, this is in a test helper for RSpec, which typically runs tests sequentially. The code does properly restore the original stdout. The risk is real but may be acceptable for the testing context. I may be underestimating the likelihood of concurrent test execution. Some teams do run RSpec in parallel. While parallel test execution exists, RSpec's parallelization happens at the process level, not thread level, so each test process would have its own $stdout. The comment raises a valid concern but is likely overly cautious for this specific RSpec testing context where process-level parallelization is more common than thread-level.

Workflow ID: wflow_SDVDmL5nNwazctp6

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 28c7483 in 34 seconds. Click for details.
  • Reviewed 34 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. demo.rb:414
  • Draft comment:
    New 'Output Matchers' category added. Verify that the category name and ordering match your intended grouping for failing tests. The trailing comma is acceptable in Ruby.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. spec/oj_serialization_spec.rb:42
  • Draft comment:
    Removed Proc serialization tests. Ensure that capturing proc output (instead of calling it) is documented and that tests reflect the new intended behavior.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_qq4h7gCaSCJPsOg1

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@swaathi swaathi merged commit 387229d into main Aug 5, 2025
3 checks passed
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