Skip to content
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

Add tests for send_file and redirect_to instrumentation #49442

Merged
merged 1 commit into from
Oct 1, 2023

Conversation

p8
Copy link
Member

@p8 p8 commented Sep 30, 2023

It seems these weren't tested before.

Caching instrumentation is tested in a similar way:

def test_fragment_cache_instrumentation
payload = nil
subscriber = proc do |_, _, _, _, event_payload|
payload = event_payload
end
ActiveSupport::Notifications.subscribed(subscriber, "read_fragment.action_controller") do
get :inline_fragment_cached
end
assert_equal "functional_caching", payload[:controller]
assert_equal "inline_fragment_cached", payload[:action]
end

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actionpack label Sep 30, 2023
@p8 p8 force-pushed the actionpack/notifications-tests branch from 8672b58 to 989de53 Compare September 30, 2023 15:23
@rafaelfranca rafaelfranca merged commit 7c303b9 into rails:main Oct 1, 2023
4 checks passed
@p8 p8 deleted the actionpack/notifications-tests branch October 2, 2023 00:10
payload = nil

subscriber = proc do |*args|
event = ActiveSupport::Notifications::Event.new(*args)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's minor but in #49433 I was hoping people would move away from this pattern ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Bedankt @bdewater 😄 #49467

@p8 p8 mentioned this pull request Oct 3, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants