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 before hooks and a function instance to Example #1750

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

parsonsmatt
Copy link
Contributor

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Copy link
Contributor Author

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

While this code is useful, it's not as useful as I'd like it to be. Not sure if we want to go forward with this implementation or do a bigger refactoring.

Comment on lines +1625 to +1636
instance YesodDispatch site => Hspec.Example (r -> SIO (YesodExampleData site) a) where
type Arg (r -> SIO (YesodExampleData site) a) =
(TestApp site, r)

evaluateExample example params action =
Hspec.evaluateExample
(action $ \(testApp, r) -> do
yed <- yesodExampleDataFromTestApp testApp
_ <- evalSIO (example r) yed
return ())
params
($ ())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This lets us use beforeApp and provide the value created as a lambda.

beforeApp (pure "asdf") $ do
    it "has asdf" $ \asdf -> do
        pure () :: YesodExample site ()
        liftIO $ asdf `shouldBe` "asdf"

, yedResponse = Nothing
}
yed <- yesodExampleDataFromApp site
evalSIO y yed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, a problem with the current implementation (as surfaced in the pending test), is that we can't persist state from the YesodExampleData that is generated. We're doing evalSIO y yed and throwing away the modified state.

This means our beforeApp and beforeWithApp hooks can modify the database and return database entities, but those entities (and any changes made to the app, like sessions or logins) won't be kept.

A proper fix would probably thread the YesodExampleData through the whole thing, and then only let it get cleared out in the Example instance.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked in depth yet, but would using a mutable variable be a solution here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SIO stuff is already a mutable variable. We're just not threading it through and re-using it afterwards.

describe "Root Route" $ do
beforeApp (get ("/" :: Text)) $ do
it "can do stuff" $ \() -> do
liftIO $ pendingWith "This test currently fails because the SIO type can't share the state. When we do an `evalSIO` in `beforeApp`, that throws away all the changes made, so requests are not persisted."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sad face

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