Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

E2e test fixes #54

Merged
merged 8 commits into from
Oct 1, 2023
Merged

E2e test fixes #54

merged 8 commits into from
Oct 1, 2023

Conversation

igiloh-pinecone
Copy link
Contributor

Problem

Multiple problems:

  1. TestClient was not used as context manager, so it didn't actually start the app (didn't send startup event)
  2. Bugs in the retry mechanism
  3. No index name randomization
  4. single test_ function instead of separate test case per app route

Solution

Use proper fixtures and context manager.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

We need to use a context manager on `TestClient` (call it in a `with()` cluase) to properly start the app inside
Used fixtures to correctly set the a random index name, and a fixture for tearing it down even if there was a failure
The retry needs to retry the /query operation itself.
Copy link
Contributor

@miararoy miararoy left a comment

Choose a reason for hiding this comment

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

lgtm, can merge

def test_upsert(client):
# Upsert a document to the index
upsert_response = client.post("/context/upsert", json=upsert_payload.dict())
assert upsert_response.is_success
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're separating the test so each one should have at least 1 logical test and not just operational (is_success)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How? There's no return value for upsert...

chat_response_content = chat_response_as_json["choices"][0]["message"][
"content"
]
print(chat_response_content)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this for debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're asking me...?
That's your code!

This way it will run from any working directory
@igiloh-pinecone igiloh-pinecone merged commit f5ae635 into v0.1-e2e-tests Oct 1, 2023
4 checks passed
@igiloh-pinecone igiloh-pinecone deleted the e2e-test-fixes branch October 1, 2023 21:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants