Skip to content

Nit: rspec describ matches class under test#2718

Merged
aramprice merged 1 commit intomainfrom
fix-describe
Apr 27, 2026
Merged

Nit: rspec describ matches class under test#2718
aramprice merged 1 commit intomainfrom
fix-describe

Conversation

@aramprice
Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings April 27, 2026 22:19
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: cc2019af-b483-4732-a494-4de0fb7af60f

📥 Commits

Reviewing files that changed from the base of the PR and between 0632952 and befa76a.

📒 Files selected for processing (1)
  • src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb

Walkthrough

The unit spec file src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb was updated. The RSpec describe label was changed to Bosh::Monitor::Plugins::EventLogger (replacing the previous Resurrector label). The test director endpoint uri was switched from http:// to https://, which updated the computed status_uri/event URLs used by request stubs and expectations. No other test control flow, setup, stubbing, or assertions were modified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request has no description provided, missing all required template sections including 'What is this change about?', contextual information, test details, and release notes guidance. Add a comprehensive pull request description following the repository template, including the change rationale, testing performed, and release notes entry.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly indicates the change is updating an rspec describe block to match the actual class under test (EventLogger), which aligns with the file changes shown in the summary.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-describe

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates an RSpec example group label to match the plugin class actually under test, improving spec clarity and avoiding confusion when reading failures.

Changes:

  • Fixes the top-level describe from Resurrector to EventLogger.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai 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

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb (1)

33-63: ⚠️ Potential issue | 🟡 Minor

Reduce flakiness: separate test time fixture from alert's auto-generated timestamp.

The test uses let(:time) { Time.new } (line 27) to construct an expected JSON string with time.to_i, but the alert fixture is created via alert_payload(...) which includes created_at: Time.now (spec_helper.rb line 37)—evaluated at a different moment. When EventLogger#process uses alert.attributes['created_at'] to build the actual request, it will have a different timestamp than the test expects. This causes intermittent failures around second boundaries.

Fix by ensuring the alert's created_at matches the test's expected time:

Example: freeze time for both test and fixture
   let(:time) { Time.new }
-  let(:alert) { Bosh::Monitor::Events::Base.create!(:alert, alert_payload(deployment: 'd', job: 'j', instance_id: 'i')) }
+  let(:alert) do
+    Timecop.freeze(time) do
+      Bosh::Monitor::Events::Base.create!(:alert, alert_payload(deployment: 'd', job: 'j', instance_id: 'i'))
+    end
+  end

Alternatively, explicitly pass created_at to alert_payload:

-  let(:alert) { Bosh::Monitor::Events::Base.create!(:alert, alert_payload(deployment: 'd', job: 'j', instance_id: 'i')) }
+  let(:alert) { Bosh::Monitor::Events::Base.create!(:alert, alert_payload(deployment: 'd', job: 'j', instance_id: 'i', created_at: time)) }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb` around
lines 33 - 63, The test is flaky because the expected timestamp (let(:time)) and
the alert fixture's created_at (from alert_payload which uses Time.now) can
differ; update the test so the alert fixture uses the same time as the
expectation by passing created_at: time into alert_payload (or freeze time for
the example) so that plugin.process(alert) will serialize the same timestamp the
test expects; locate the alert construction that calls alert_payload and the
test's let(:time) to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb`:
- Around line 33-63: The test is flaky because the expected timestamp
(let(:time)) and the alert fixture's created_at (from alert_payload which uses
Time.now) can differ; update the test so the alert fixture uses the same time as
the expectation by passing created_at: time into alert_payload (or freeze time
for the example) so that plugin.process(alert) will serialize the same timestamp
the test expects; locate the alert construction that calls alert_payload and the
test's let(:time) to make this change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0ed3fa8b-1a1d-4e24-980e-02190f35b71a

📥 Commits

Reviewing files that changed from the base of the PR and between 2c70a0d and 0632952.

📒 Files selected for processing (1)
  • src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb

coderabbitai[bot]
coderabbitai Bot previously approved these changes Apr 27, 2026
@github-project-automation github-project-automation Bot moved this from Inbox to Pending Merge | Prioritized in Foundational Infrastructure Working Group Apr 27, 2026
- update example to use tls
@aramprice aramprice merged commit 9b93b34 into main Apr 27, 2026
22 checks passed
@aramprice aramprice deleted the fix-describe branch April 27, 2026 22:51
@github-project-automation github-project-automation Bot moved this from Pending Merge | Prioritized to Done in Foundational Infrastructure Working Group Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants