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

Add testcases for GET /dsmetadata API #1199

Merged
merged 13 commits into from
Jun 18, 2024

Conversation

shreyabiradar07
Copy link
Contributor

@shreyabiradar07 shreyabiradar07 commented May 27, 2024

Description

This PR adds below testcases for GET /dsmetadata API

  • List dsmetadata specifying a valid datasource

  • List dsmetadata for a datasource with parameters by specifying the following parameters:

    • /dsmetadata?datasource=<datasource_name>&verbose=false
    • /dsmetadata?datasource=<datasource_name>&verbose=true
    • /dsmetadata?datasource=<datasource_name>&cluster_name=<cluster_name>&verbose=false
    • /dsmetadata?datasource=<datasource_name>&cluster_name=<cluster_name>&verbose=true
    • /dsmetadata?datasource=<datasource_name>&cluster_name=<cluster_name>&namespace=<namespace_name>&verbose=false
    • /dsmetadata?datasource=<datasource_name>&cluster_name=<cluster_name>&namespace=<namespace_name>&verbose=true
  • List dsmetadata with invalid parameter values for datasource, cluster_name and namespace

    • Non-existing datasource
    • Non-existing cluster_name
    • Non-existing namespace
  • List dsmetadata without specifying any parameters

  • List dsmetadata after creating a datasource but without importing metadata

  • List dsmetadata with datasource and namespace but without cluster_name

  • List the dsmetadata after deleting imported metadata

  • List the dsmetadata for a datasource that has been deleted

Fixes # 1129

Type of change

  • Bug fix
  • New feature
  • Docs update
  • Breaking change (What changes might users need to make in their application due to this PR?)
  • Requires DB changes
  • New Tests
  • Updates to existing tests

How has this been tested?

  • New Test X
  • Functional testsuite - Tested using local monitoring testsuite
  • Functional test output on Jenkins - 89 with above testcases

Test Configuration

  • Kubernetes clusters tested on: minikube

Checklist 🎯

  • Followed coding guidelines
  • Comments added
  • Dependent changes merged
  • Documentation updated
  • Tests added or updated

Additional information

NA

@shreyabiradar07 shreyabiradar07 self-assigned this May 27, 2024
@shreyabiradar07 shreyabiradar07 added test kruize-local Tag for mentioning all the PR's and issues raised which covers the kruize local monitoring usecase labels May 27, 2024
print("Response status code = ", response.status_code)
print("\n************************************************************")
print(response.text)
Copy link
Contributor

Choose a reason for hiding this comment

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

Include a logging parameter to this function and enable this when it is true, this will help us enable/disable based on the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@shreyabiradar07 shreyabiradar07 marked this pull request as ready for review May 28, 2024 13:53
@@ -50,6 +50,9 @@
PERFORMANCE_RECOMMENDATIONS_AVAILABLE = "Performance Recommendations Available"
CONTAINER_AND_EXPERIMENT_NAME = " for container : %s for experiment: %s.]"
LIST_DATASOURCES_ERROR_MSG = "Given datasource name - \" %s \" either does not exist or is not valid"
LIST_METADATA_DATASOURCE_NAME_ERROR_MSG = "Metadata for a given datasource name - \" %s \" either does not exist or is not valid"
LIST_METADATA_ERROR_MSG = ("Metadata for a given datasource - \" %s \", cluster name - \" %s \", namespace - \"%s \" "
Copy link
Contributor

Choose a reason for hiding this comment

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

Have different error messages for each of the scenarios

from helpers.list_metadata_json_cluster_name_without_verbose_schema import *
from jinja2 import Environment, FileSystemLoader

@pytest.mark.sanity
Copy link
Contributor

Choose a reason for hiding this comment

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

@shreyabiradar07 - Can you share the test results or link to the run

Copy link
Contributor Author

Choose a reason for hiding this comment

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


list_metadata_json = response.json()
assert response.status_code == ERROR_STATUS_CODE
cluster_name = "null"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need cluster name & namespace here as the list metadata is only with datasource

@pytest.mark.parametrize("test_name, expected_status_code, datasource",
[
("blank_datasource", 400, ""),
("null_datasource", 400, "null"),
Copy link
Contributor

Choose a reason for hiding this comment

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

null shouldn't be a string here, can you cross check with create experiment negative tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test_type = {"blank": "", "null": "null", "invalid": "xyz"}
used in generate_test_data for generating negative scenarios in create experiment tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to strip the double quotes but still null is treated as a string - this might be because datasource here is an input query parameter
Also tried passing None but the parameter itself is getting omitted. Will further look for alternatives for null in Python

datasource = json_data['datasource_name']
# Currently only default cluster is supported by Kruize
cluster_name = "default"
namespace = "monitoring"
Copy link
Contributor

Choose a reason for hiding this comment

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

Namespace monitoring would only work on minikube. Has this been tested on openshift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

In mandatory parameters missing test, there is a change required in the error message

Mandatory parameters missing [dataSourceName]  - this should be datasource_name

Copy link
Contributor

Choose a reason for hiding this comment

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

delete metadata returns 405 instead of 200, can you please check this

Deleting the metadata...
URL =  http://192.168.49.2:31830/dsmetadata
<Response [405]>
Response status code =  405
delete metadata =  405

Copy link
Contributor

Choose a reason for hiding this comment

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

In the test, rest_apis/test_import_metadata.py::test_repeated_metadata_import, introduce some changes by creating new namespaces and deleting one of the namespace and validate if the second import output reflects these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In mandatory parameters missing test, there is a change required in the error message

Mandatory parameters missing [dataSourceName] - this should be datasource_name

dataSourceName variable is referred from the DSMetadataAPIObject field during validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

delete metadata - 405 and current test failures - test_list_metadata_after_deleting_metadata & test_list_metadata_without_import_action will be resolved once PR 1178 is merged

Copy link
Contributor

Choose a reason for hiding this comment

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

In the test, rest_apis/test_import_metadata.py::test_repeated_metadata_import, introduce some changes by creating new namespaces and deleting one of the namespace and validate if the second import output reflects these changes.

Are these changes made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated test rest_apis/test_import_metadata.py::test_repeated_metadata_import by dynamically creating and deleting namespaces to showcase the delete metadata API functionality

test log output test_repeated_metadata_import.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated test logs for rest_apis/test_import_metadata.py::test_repeated_metadata_import
updated_test_repeated_metadata_import.txt

@pytest.mark.negative
def test_list_metadata_without_import_action(cluster_type):
"""
Test Description: This test validates GET /dsmetadata without importing metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Add expected behavior as well to the description


delete_namespace("repeated-metadata-import")
delete_namespace("local-monitoring-test")
Copy link
Contributor

Choose a reason for hiding this comment

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

EOF missing, fix wherever applicable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated missing EOF

Copy link
Contributor

@chandrams chandrams left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@chandrams chandrams left a comment

Choose a reason for hiding this comment

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

LGTM

@dinogun dinogun merged commit cfadfe0 into kruize:mvp_demo Jun 18, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kruize-local Tag for mentioning all the PR's and issues raised which covers the kruize local monitoring usecase test
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants