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

Transform Main.hs into Tests #1

Merged
merged 4 commits into from
Jul 23, 2020
Merged

Transform Main.hs into Tests #1

merged 4 commits into from
Jul 23, 2020

Conversation

voidus
Copy link

@voidus voidus commented Jun 19, 2020

Hi 👋

I started converting the Main.hs from the checklist in the main PR into tests. I don't have a lot of experience with larger Haskell projects, so any feedback is appreciated.

voidus added 3 commits June 19, 2020 23:49
This is a non-ideal implementation which relies on UndecidableInstances and
orphan instances.
This avoids UndecidableInstances and TypeApplications in favor of simple
newtypes.
@voidus
Copy link
Author

voidus commented Jun 19, 2020

My understanding is that this can only be reasonably tested in the servant-server and servant-client packages. I was peeking at type-spec and tao, but I don't think this would be the place to introduce them, even if there was consensus on that.

For now this is only the server part, I'll look into the client part as soon as I find time.

CONTRIBUTING.md mentioned -Werror-compatibility to check ghc 7.8 and 7.10 compatibility. I searched the web, but couldn't figure out what it means. I ran stylish and hlint though.

@voidus
Copy link
Author

voidus commented Jun 24, 2020

I think I found an issue with the current implementation. servant-client interprets responses with status 301 as an error and gives us a Left ClientError even if we have that response defined in the API, which isn't what I would expect.

I added a test for the current behaviour and a comment, since I think changing that is out of scope for this PR.

@voidus voidus marked this pull request as ready for review June 24, 2020 12:17
@voidus
Copy link
Author

voidus commented Jun 24, 2020

BTW, the tests for servant-http-streams seem to be broken or need some more setup that I can't figure out 😐

@voidus
Copy link
Author

voidus commented Jul 19, 2020

@fisx highlighting you since you seem to have done most work on the uverb branch.

@fisx
Copy link

fisx commented Jul 21, 2020

thanks! i'll take a look at this soon, very happy to see that you're picking this up! :)

@fisx fisx mentioned this pull request Jul 23, 2020
16 tasks
Copy link

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I left some nit-picks, but I'll merge this anyway because it's a step towards checking another box in the main PR. Up to you if you want to open another PR.

I agree and left this as a new check-box in the main PR.

I also agree that there isn't much to test in the servant package.

I couldn't find anything on -Werror-compatible either, so I suppose as long as we're passing the CI it's fine. :)

:<|> emptyServer
:<|> (\shouldRedirect -> if shouldRedirect
then respond (WithStatus @301 ("redirecting" :: Text))
else respond (WithStatus @200 alice ))
Copy link

Choose a reason for hiding this comment

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

Suggested change
else respond (WithStatus @200 alice ))
else respond (WithStatus @200 alice))

Comment on lines +169 to +176
-- Right response ->
-- case response of
-- Z (Identity (WithStatus person :: WithStatus 200 Person)) ->
-- fail "Expected to be redirected"
-- S (last) ->
-- let Identity (WithStatus message :: WithStatus 301 Text)
-- = unZ last
-- in message `shouldBe` "redirecting"
Copy link

Choose a reason for hiding this comment

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

Suggested change
-- Right response ->
-- case response of
-- Z (Identity (WithStatus person :: WithStatus 200 Person)) ->
-- fail "Expected to be redirected"
-- S (last) ->
-- let Identity (WithStatus message :: WithStatus 301 Text)
-- = unZ last
-- in message `shouldBe` "redirecting"
-- Right (Z (Identity (WithStatus person :: WithStatus 200 Person)) ->
-- fail "Expected to be redirected"
-- Right (S last) ->
-- let Identity (WithStatus message :: WithStatus 301 Text) = unZ last
-- in message `shouldBe` "redirecting"

(you could collapse the nexted cases in the actual test as well if you like, but i agree, that code shouldn't make it out of the uverb-branch.)

@fisx fisx merged commit 453619d into zerobuzz:uverb Jul 23, 2020
@fisx
Copy link

fisx commented Jul 23, 2020

Thanks again!

fisx pushed a commit that referenced this pull request Jul 31, 2020
* Create tests from the Example.hs
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