Skip to content

Conversation

@pierrevalade
Copy link
Contributor

@guillaq I think something's off is going on with the run_url method. I spent some time trying to fix the issue, but wasn't successful.

But I think I've added the tests that should fail until the bug is resolved.

@pierrevalade pierrevalade requested a review from guillaq February 26, 2025 17:17
@pierrevalade
Copy link
Contributor Author

    def test_run_url_no_api_url(self, run1: Run[_TestOutput]):
        # When WORKFLOWAI_API_URL is not set, the app URL should default to workflowai.com
>       assert run1.run_url == "https://workflowai.com/_/agents/agent-id/runs/run-id"
E       AssertionError: assert 'None/_/agent...d/runs/run-id' == 'https://work...d/runs/run-id'
E         
E         - https://workflowai.com/_/agents/agent-id/runs/run-id
E         + None/_/agents/agent-id/runs/run-id

workflowai/core/domain/run_test.py:153: AssertionError

@guillaq
Copy link
Collaborator

guillaq commented Feb 26, 2025

I think the issue is in the way the test is written.
It is creating a situation that could never exist.

Because the values are described as

WORKFLOWAI_APP_URL = os.getenv("WORKFLOWAI_APP_URL", _default_app_url())

WORKFLOWAI_API_KEY = os.getenv("WORKFLOWAI_API_KEY", "")

WORKFLOWAI_APP_URL and WORKFLOWAI_API_KEY will never be None.

To test specific environment variables, you would need to patch the environment BEFORE the file env.py is imported.

I had found this issue earlier today that I pushed like a savage. I'll push a test in this PR.

@pierrevalade
Copy link
Contributor Author

I had found this issue earlier today that I pushed like a savage. I'll push a test in this PR.

yes, add the test too! :)


OK I'll wait.

@guillaq
Copy link
Collaborator

guillaq commented Feb 26, 2025

The reason I did not write the test is that it is somewhat hard to write...
You need to patch the environment before the files are imported. My quick attempts just failed... I'll check again later !

Copy link
Contributor Author

ha OK — then write a comment in the modified file 🙂 always explain your thinking.

@guillaq
Copy link
Collaborator

guillaq commented Feb 27, 2025

Had to reload the modules with importlib.reload.

I checked and it catches the error I fixed earlier (8842dee)

@pierrevalade pierrevalade marked this pull request as ready for review February 27, 2025 03:28
@pierrevalade
Copy link
Contributor Author

thank you @guillaq -- ready! could you please push a new version of the pip package?

@guillaq guillaq merged commit 64db518 into main Feb 27, 2025
5 checks passed
@guillaq guillaq deleted the pierre/run-url branch February 27, 2025 14:04
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