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 test on handler priority #373

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

cblp
Copy link
Contributor

@cblp cblp commented Feb 10, 2024

No description provided.

@ocramz
Copy link
Collaborator

ocramz commented Feb 11, 2024

@cblp Thanks, but we can't merge code if it breaks CI. If you want to show a problem with the current implementation, could you make a dedicated subsection in the tests ("context"..) and rewrite tests such that they pass?

@cblp
Copy link
Contributor Author

cblp commented Feb 11, 2024

I can't make it pass without changing main implementation.

@cblp
Copy link
Contributor Author

cblp commented Feb 11, 2024

@ocramz what do you think of this implementation?

@ocramz ocramz self-requested a review February 22, 2024 06:18
Copy link
Collaborator

@ocramz ocramz left a comment

Choose a reason for hiding this comment

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

I would like some new tests that clarify the need for the new exception handler priority.

@@ -111,9 +111,9 @@ runAction :: MonadUnliftIO m =>
-> m (Maybe Response)
runAction mh env action = do
ok <- flip runReaderT env $ runAM $ tryNext $ action `catches` concat
[ [actionErrorHandler]
[ [actionErrorHandler, statusErrorHandler, scottyExceptionHandler]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes the behaviour of the library, but it's not caught in the tests.

@cblp
Copy link
Contributor Author

cblp commented Mar 10, 2024

I'm not sure how to write such tests, I did it intuitively. I just want to see exception when it is thown from a handler.

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