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 Rack::Lint to ActionDispatch::ShowExceptions tests #48842

Conversation

adrianna-chang-shopify
Copy link
Contributor

Motivation / Background

This wraps test coverage for ActionDispatch::ShowExpections in Rack::Lint middleware in order to validate that both ActionDispatch::ShowExceptions and ActionDispatch::PublicExceptions conform to the Rack SPEC.

It also ensures that the response headers returned by the *Exceptions middleware respect casing (mixed case for Rack 2, lower case for Rack 3).

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 Jul 28, 2023
@guilleiguaran guilleiguaran self-requested a review July 28, 2023 18:48
@adrianna-chang-shopify
Copy link
Contributor Author

Test failure is due to a hardcoded X-Cascade header being set in actionpack/lib/action_dispatch/journey/router.rb here:

[404, { "X-Cascade" => "pass" }, ["Not Found"]]

Does #47108 still make sense to merge ahead of adding Rack::Lint to all of the ActionDispatch::*Exceptions middleware tests?

@skipkayhil
Copy link
Member

I think we should just fix the header here. The conditional header strategy should obsolete #47108

@adrianna-chang-shopify
Copy link
Contributor Author

Done 👍 Happy to pull the router-related changes out to a separate PR if that would be more appropriate.

@adrianna-chang-shopify
Copy link
Contributor Author

Ah, it's going to require changes to more than just ActionDispatch::Journey::Router -- there are more test failures 😅 I'm going to open the changes in a separate PR.

@adrianna-chang-shopify
Copy link
Contributor Author

#48845

@guilleiguaran guilleiguaran self-assigned this Jul 30, 2023
This wraps test coverage for `ActionDispatch::ShowExpections` in
`Rack::Lint` middleware in order to validate that both
`ActionDispatch::ShowExceptions` and `ActionDispatch::PublicExceptions`
conform to the Rack SPEC.

It also ensures that the response headers returned by the *Exceptions
middleware respect casing (mixed case for Rack 2, lower case for Rack 3)
@adrianna-chang-shopify
Copy link
Contributor Author

@guilleiguaran this should be ready to go now, thanks!

@guilleiguaran guilleiguaran merged commit 1ffdaec into rails:main Aug 1, 2023
9 checks passed
@skipkayhil
Copy link
Member

skipkayhil commented Aug 1, 2023

Ref #48874

(linking to keep everything together)

@adrianna-chang-shopify adrianna-chang-shopify deleted the ac-rack-lint-show-exceptions branch August 1, 2023 18:05
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