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

🐛 Stream test bug fix #539

Merged
merged 5 commits into from Oct 26, 2023
Merged

Conversation

prashantgupta24
Copy link
Contributor

@prashantgupta24 prashantgupta24 commented Oct 20, 2023

What this PR does / why we need it:

This PR adds:

fix #540

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@joerunde
Copy link
Collaborator

... so that it's used within a context manager.

But all the tests shown already were using it within a context manager? 😕😕😕

It looks like what this is doing is really moving to a single TestClient instead of having one per test. I'm totally down for this anyway since it's much DRYer but still confused about the root cause here.

@prashantgupta24
Copy link
Contributor Author

@joerunde yeah the only difference was the session scoping of the fixture that was making it work. It doesn't work without that unfortunately :(

Copy link
Collaborator

@joerunde joerunde left a comment

Choose a reason for hiding this comment

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

Honestly, I'm here for the test code simplification, the fixture for the client is really nice. Let us know if you find out any more details about why this thing hangs!

@prashantgupta24
Copy link
Contributor Author

@joerunde Now I'm thinking making the fixture session-scoped is not the right thing to do. Adding another test that pings an actual running server and using that along with either a function scoped or a session scoped test fixture doesn't seem to work :(

My gut feeling is that TestClient and EventSourceResponse don't seem to work well together. If I don't use TestClient, then running any number of tests against the stream endpoint seem to work.

@prashantgupta24 prashantgupta24 marked this pull request as draft October 24, 2023 22:08
Signed-off-by: Prashant Gupta <prashantgupta24@gmail.com>
Signed-off-by: Prashant Gupta <prashantgupta24@gmail.com>
Signed-off-by: Prashant Gupta <prashantgupta24@gmail.com>
Signed-off-by: Prashant Gupta <prashantgupta24@gmail.com>
Signed-off-by: Prashant Gupta <prashantgupta24@gmail.com>
@prashantgupta24 prashantgupta24 marked this pull request as ready for review October 25, 2023 21:55
@prashantgupta24 prashantgupta24 merged commit 9ca09a1 into caikit:main Oct 26, 2023
8 checks passed
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.

Stream tests not working if 2 are run in sequence
3 participants