Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new function Expect.runExpectation that allows running an Expectation directly in IO context. This functionality is needed to integrate with the Llm.Test.Cached API from the llm-client library.
Changes:
- Added
runExpectationfunction inTest.Internalthat executes expectations in IO, returning results on success or throwingFailureexceptions on failure - Exported the new function through the
Expectmodule - Added comprehensive test coverage for the new function
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| nri-prelude/src/Test/Internal.hs | Implements runExpectation function that converts expectations to IO actions, with proper error handling via exception throwing |
| nri-prelude/src/Expect.hs | Exports the new runExpectation function in the public API |
| nri-prelude/tests/TestSpec.hs | Adds test suite for runExpectation covering success and failure cases |
| nri-prelude/tests/golden-results-/test-report- | Updates golden test files with adjusted line numbers due to test addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
nri-prelude/src/Test/Internal.hs
Outdated
| -- On success, returns the result. On failure, throws the Failure as an exception. | ||
| runExpectation :: Expectation' a -> Prelude.IO a | ||
| runExpectation expectation = do | ||
| log <- Platform.silentHandler |
There was a problem hiding this comment.
I'm a bit worried about using silentHandler here. That will just not log anything that we do inside of our test setup which is probably not what we want.
Let's modify this function to take LogHandler as an argument and then use that logger for Task.perform.
Inside of withMocks we should be able to call Expect.succeeds Platform.logHandler to get the handler to use with runExpectation.
micahhahn
left a comment
There was a problem hiding this comment.
Actually hold on a second... this exception test wasn't exactly what I was thinking
| Internal.fromIO, | ||
| Internal.runExpectation, | ||
| Internal.fromIOResult, | ||
| Internal.Expectation', | ||
| around, |
There was a problem hiding this comment.
I believe we will also need to expose Failure right?
micahhahn
left a comment
There was a problem hiding this comment.
Alright, now we are looking good to merge
This adds a new function
Expect.runExpectation, which takes an Expectation and runs it into anIO. This is necessary for using theLlm.Test.CachedAPI from thellm-clientlibrary we now have.