langchainrb icon indicating copy to clipboard operation
langchainrb copied to clipboard

Rspec 'integration' tests for optional use by contributors and ci

Open mattlindsey opened this issue 1 year ago • 10 comments

This isn't done, but I'm looking for feedback on using either cucumber or rspec for integration tests, which would aid in BDD development and be run before releases. If rspec is chosen, we can configure it to skip the 'integration' folder unless requested.

This PR includes both options for comparison.

For #144

One difficulty here is I am getting several different answers for the tests, so what's the acceptance criteria? Just absence of errors? 5,845 soccer fields 204 soccer fields 4,125 soccer fields

mattlindsey avatar Jun 07 '23 15:06 mattlindsey

@mattlindsey Isn't RSpec enough for testing library code? Why do we need cucumber it doesn't seem to bring additional value to what we can already do with rspec testing an Agent.

alchaplinsky avatar Jun 08 '23 13:06 alchaplinsky

@alchaplinsky Yeah. I'll redo this proposal to only include the rspec file, exclude the 'integrations' folder when running rspec, and add a short note in the README suggesting contributors utilize the specs directly.

mattlindsey avatar Jun 08 '23 13:06 mattlindsey

@alchaplinsky. Is this better? I think they're useful.

mattlindsey avatar Jun 08 '23 16:06 mattlindsey

@mattlindsey I think integration tests for concept like Agent is a good idea. But we need to mock requests to external APIs. I don't think that anyone would want to pay for tokens used during test runs.

alchaplinsky avatar Jun 08 '23 16:06 alchaplinsky

Personally I wanted these to aid in development, but I can just keep them for myself. Existing tests already do the mocking.

mattlindsey avatar Jun 08 '23 16:06 mattlindsey

@alchaplinsky Sorry, I meant to tag you on the last comment. I added a line in the README to clarify the intent of the directory and files.

mattlindsey avatar Jun 08 '23 19:06 mattlindsey

Ok, got it. So this is a type of test that actually runs against real APIs and makes sure that code works with real integrations. Thus we skip running them every time when we run a test suite and we can enable them by setting the INTEGRATION env variable. Is that right? I'd probably name that variable something more descriptive like: INTEGRATION_TESTING = true INTEGRATION_TESTS_ENABLED = true

Otherwise, it is not entirely clear what INTEGRATION stands for.

alchaplinsky avatar Jun 09 '23 08:06 alchaplinsky

INTEGRATION_TESTS_ENABLED = true

@alchaplinsky Yeah. That's better. Done

mattlindsey avatar Jun 09 '23 09:06 mattlindsey

Closing this to avoid too many open PRs but leaving issue #144 open.

mattlindsey avatar Jun 10 '23 17:06 mattlindsey

I'm reopening this PR because I think it's a good idea to have these optional 'integration' tests, especially for use when developing langchainrb itself. It's quicker and less error prone than using bin/console.

@andreibondarev I can update this PR to test Langchain::Assistant instead of ChainOfThought if you agree this might be useful.

mattlindsey avatar Jan 09 '24 16:01 mattlindsey