-
Notifications
You must be signed in to change notification settings - Fork 752
chore: refactor testing to use mock HTTP responses #413
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
dde81f7
to
c2c5981
Compare
c2c5981
to
a90fdbf
Compare
8499edb
to
252caed
Compare
e8739f5
to
df1e0f0
Compare
57520a6
to
2292019
Compare
b28cc74
to
e0576f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
While I believe mocking is a much better option here, it will be far more complex to implement, and the suggested solution will introduce tremendous improvement in the short term.
@@ -31,3 +32,9 @@ def joke_workflow(): | |||
"joke_creation.task", | |||
"pirate_joke_generator.workflow", | |||
] | |||
open_ai_span = spans[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spans[0]
? I would look for span by name.
assert ( | ||
open_ai_span.attributes["llm.prompts.0.content"] | ||
== "Tell me a joke about opentelemetry" | ||
) | ||
assert open_ai_span.attributes.get("llm.completions.0.content") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the order seems off, doesn't it make more sense to assert the the value exists first?
Move tests for specific instrumentations to be under that instrumentation directory, and refactor to use VCR.py