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

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

Closed
wants to merge 12 commits into from

Conversation

mattlindsey
Copy link
Contributor

@mattlindsey mattlindsey commented Jun 7, 2023

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

@alchaplinsky
Copy link
Contributor

@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.

@mattlindsey
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@alchaplinsky
Copy link
Contributor

@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.

@mattlindsey
Copy link
Contributor Author

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

@mattlindsey mattlindsey changed the title Two choices for integration testing Rspec 'integration' tests for optional use by contributors and ci Jun 8, 2023
@mattlindsey
Copy link
Contributor Author

@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.

@alchaplinsky
Copy link
Contributor

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.

@mattlindsey
Copy link
Contributor Author

INTEGRATION_TESTS_ENABLED = true

@alchaplinsky Yeah. That's better. Done

@mattlindsey
Copy link
Contributor Author

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

@@ -13,6 +13,9 @@
# Disable RSpec exposing methods globally on `Module` and `main`
config.disable_monkey_patching!

# Only run integration tests when enabled
config.filter_run_excluding type: :integration unless ENV["INTEGRATION_TESTS_ENABLED"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could probably just filter out all integration tests by default is ENV["CI"] == true

)
result = agent.run(question: question)

expect(result).to include("distance between NYC and DC")
Copy link
Collaborator

Choose a reason for hiding this comment

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

One way that people do this is indexing into a vectorsearch DB all of the variations of possible acceptable answers and then run a vector search agains them to see if they "match".

@mattlindsey
Copy link
Contributor Author

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 mattlindsey reopened this Jan 9, 2024
@mattlindsey mattlindsey marked this pull request as draft January 9, 2024 19:09
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.

3 participants