Fix exception-swallowing code path

[dandavison]

Fixes #624

This is a bug identified by pyright type checking.

How this was tested

• PR includes two commits: a test that fails on main before the fix, and the fix.

I also confirmed manually that if the server responds with gRPC error ALREADY_EXISTS, but the error details do not satisfy the logic in the SDK code, then the user gets

UnboundLocalError: cannot access local variable 'resp' where it is not associated with a value

What they should get is temporalio.service.RPCError.

[cretz]

👍 I saw this one the other day, but I don't remember the context or why I didn't create the issue (or can't find it)

[dandavison]

Added a test based on monkey-patching the workflow_service method. PR has two commits:

• The failing test
• A fix

Ready for review.

[cretz]

Shouldn't need to monkey patch, should be easy enough to make whichever code path occur against a real server

[dandavison]

Shouldn't need to monkey patch, should be easy enough to make whichever code path occur against a real server

Actually, monkey patching is more appropriate in this case. What we're trying to assert is that if the client receives an ALREADY_EXISTS gRPC error, it will always be raised, even if its details aren't of a particular form. There's no need for that to be an integration test involving a real server, and there are lots of arguments for it not to be (test suite execution speed, encouraging good test suite style, ease of understanding the test assertion, etc). Monkey–patching is a very standard approach in Python testing: it's explicitly supported by tools in the standard library (mock.patch).

[cretz]

I personally prefer the end to end integration test, and not too concerned about speed (we have an already-exists integration test already I think). We have dozens of tests using the server harmlessly to test actual client behavior in actual situations without advanced knowledge of inner workings. Patching is brittle IMO, but if you feel you must do it, ok.

[cretz]

EDIT: Nevermind, I misread the original code and thought we were swallowing more than just already-exist-wrong-details exceptions. Can ignore my desire for integration tests here.

[dandavison]

You're right though! The test is a bit brittle. But yes here we're in a situation where there's a code path that's technically a bug, but not obvious how to repro it with a real interaction with a server, and yet we have an obligation to put in a test to back the fix.