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

Get endpoint uri test #42

Merged
merged 11 commits into from
Apr 24, 2024
Merged

Get endpoint uri test #42

merged 11 commits into from
Apr 24, 2024

Conversation

adrianvrj
Copy link
Contributor

Added two test cases for get_endpoint_uri function, in case an endpoint is found and in case it is not.

Copy link
Contributor

@Gonmeso Gonmeso left a comment

Choose a reason for hiding this comment

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

Thank you for your input!

Couple of things:

  • You should update this branch with the latest changes in main as we have added testing in the CI
  • Run pre-commit which would format and order the imports
  • Add the data for the test inside the tests for clarity

Copy link
Contributor

Choose a reason for hiding this comment

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

This file is missing formatting, this should have been handled by pre-commit, make sure it is installed and run pre-commit run --files tests/test_utils.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already done this, you can see it at 08cf401 commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Running pre-commit locally yields changes:

Check Yaml...........................................(no files to check)Skipped
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
Check for added large files..............................................Passed
Check for merge conflicts................................................Passed
Don't commit to branch...................................................Passed
pyupgrade................................................................Passed
isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing /Users/gonmeso/src/external/actions-sdk/tests/test_utils.py

black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted tests/test_utils.py

All done! ✨ 🍰 ✨
1 file reformatted.

ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

tests/test_utils.py:1:1: F403 `from unittest.mock import *` used; unable to detect undefined names
tests/test_utils.py:3:8: F401 [*] `pytest` imported but unused
tests/test_utils.py:9:2: F405 `patch` may be undefined, or defined from star imports
tests/test_utils.py:29:2: F405 `patch` may be undefined, or defined from star imports
Found 4 errors.
[*] 1 fixable with the `--fix` option.

Run pre-commit run --files tests/test_utils.py and fix the formating and linting issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think now I get it right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

from giza_actions.utils import get_endpoint_uri

endpoint_data = Endpoint(id=999, size="S", is_active=True, model_id=999, version_id=999, uri="testing.uri")
endpoint_list = EndpointsList(root=[endpoint_data])
Copy link
Contributor

Choose a reason for hiding this comment

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

To improve readability a bit we could add this inside the test and add it to the mock:

@patch("giza.client.EndpointsClient.list")
def test_get_endpoint_uri_successful(mock_get):
    """
    Tests successful retrieval of the deployment URI for a model and version.
    """
    endpoint_data = Endpoint(id=999, size="S", is_active=True, model_id=999, version_id=999, uri="testing.uri")
    endpoint_list = EndpointsList(root=[endpoint_data])
    mock_get.return_value = endpoint_list
    uri = get_endpoint_uri(model_id=788, version_id=23)
    assert uri is "testing.uri"
    mock_get.assert_called_once()

This way data that its only for a test its contained inside and not for the whole script. Make sure to change the test_get_endpoint_uri_not_found test as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already done this too, it is the df54f72 commit

@adrianvrj
Copy link
Contributor Author

@Gonmeso I also informed the other devs in charge of this issue to make this corrections.

@@ -0,0 +1,27 @@
from unittest.mock import *
Copy link
Contributor

Choose a reason for hiding this comment

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

Importing with * is considered a bad practice and usually discouraged in lintiing tools, only import what is needed, which is patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here d3bd146

@adrianvrj adrianvrj requested a review from Gonmeso April 23, 2024 20:39
Copy link
Contributor

@Gonmeso Gonmeso left a comment

Choose a reason for hiding this comment

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

Two other tests have been added by your colleague, please update from main to add them and fix any conflict, then it will be good to go!

@adrianvrj adrianvrj requested a review from Gonmeso April 24, 2024 13:06
@Gonmeso Gonmeso self-assigned this Apr 24, 2024
Copy link
Contributor

@Gonmeso Gonmeso left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gonmeso Gonmeso merged commit b1a9e52 into gizatechxyz:main Apr 24, 2024
1 check passed
@Gonmeso Gonmeso mentioned this pull request Apr 26, 2024
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.

2 participants