-
Notifications
You must be signed in to change notification settings - Fork 10
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
Unit test AixplainClient #65
base: v2
Are you sure you want to change the base?
Conversation
@MAlyafeai18 can you please add a PR description describing the intent of the PR? |
…stAssetMixin, and GetAssetMixin Base classes
test/test_datasets.py
Outdated
dataset = Dataset(dataset_dict) | ||
|
||
# Mock the HTTP response | ||
httpretty.register_uri(httpretty.GET, urljoin(BASE_URL,'/sdk/datasets/team/?pageNumber=1&team_id=456'), status=200, body='{"items": []}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would better to conform standard flake8 checks all over the place
return [cls(item) for item in payload['items']] | ||
payload_json = payload.json() | ||
if 'items' in payload_json: | ||
key = 'items' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make key
a class level field so that subclasses can change it as per their needs explicitly, what you think? I mean how we did for asset_path
like below:
asset_path = None
paging_key = 'items'
@httpretty.activate | ||
def test_batch_upload_to_s3_dict(): | ||
upload_url = urljoin(BASE_URL, "/upload").replace('.', '-') | ||
httpretty.register_uri(httpretty.POST, urljoin(BASE_URL,'sdk/file/upload/temp-url'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd better use flake8 to ensure coding style
url = urljoin(BASE_URL,os.path.join('sdk',asset_path, 'paginate')) + "?" + urlencode(params) | ||
httpretty.register_uri(httpretty.GET, url, status=200, body = mock_response) | ||
# this need to be implemented in a different way inside the Dataset function | ||
resp = dataset.list(subpaths = ['paginate']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean pagination path is found at datasets/paginate
? if so maybe we can also treat this like what we did for asset_path
like pagination_path
or sth similar? What you think?
|
||
@pytest.fixture(autouse=True) | ||
def setup_env_variables(): | ||
os.environ['BACKEND_URL'] = 'https://api.example.com' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we got rid off auto enum generation, i think we also get rid of these auto client generation based on env vars. We have to think about this.
# Mocking the AixplainClient | ||
BASE_URL = os.getenv('BACKEND_URL', 'https://api.example.com') | ||
|
||
"""@pytest.fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove this if no need.
dataset = Dataset(dataset_dict) | ||
# Mock the HTTP response | ||
httpretty.register_uri(httpretty.GET, urljoin(BASE_URL,'/sdk/datasets/123/download'), status=200) | ||
response = dataset.download() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to find ways to mock download request, this version not makes sense much.
assert response.status_code == 200 | ||
|
||
@httpretty.activate | ||
def test_dataset_team_page(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
despite the name of the test, why we're just testing for empty page? I believe we'd better mock response here also if this is not by purpose.
assert result['status'] == "IN_PROGRESS" | ||
assert result["url"] == urljoin(MODELS_RUN_URL,"123") | ||
|
||
"""@httpretty.activate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove commented parts in case not really by purpose
url = urljoin(BASE_URL,os.path.join('sdk',asset_path)) + "?" + urlencode(params) | ||
httpretty.register_uri(httpretty.GET, url, status=200, body = mock_response) | ||
|
||
resp = model.list() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list method's main purpose is to continuesly fetching next pages within the range but here we only want it to fetch only one which is not testing the main function.
No description provided.