Add and update tests for the listRecommendations and UpdateRecommendations APIs #981

khansaad · 2023-10-12T06:52:07Z

This PR fixes #978

Update existing tests to match with the new API structure
Add notification code checks

Signed-off-by: saakhan <[email protected]>

chandrams · 2023-10-30T09:10:11Z

tests/scripts/remote_monitoring_tests/rest_apis/test_list_recommendations.py

-    assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['112101'][
-               'message'] == 'Cost Recommendations Available'
+    assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['111000'][
+               'message'] == 'Recommendations Are Available'


Please define the expected messages in utils.py and reference it wherever needed.

chandrams · 2023-10-30T11:39:56Z

tests/scripts/remote_monitoring_tests/rest_apis/test_list_recommendations.py

-                short_term_recommendation_current = None
-                if "current" in short_term_recommendation:
-                    short_term_recommendation_current = short_term_recommendation["current"]
+                if INFO_COST_RECOMMENDATIONS_AVAILABLE_CODE in short_term_recommendation["notifications"]:


As discussed, please include tests and schema changes for medium and long term recommendation validation too

chandrams · 2023-10-30T11:41:09Z

tests/scripts/remote_monitoring_tests/rest_apis/test_update_recommendations.py

@@ -227,8 +227,8 @@ def test_update_valid_recommendations_just_endtime_input_after_results_after_cre
        data = response.json()
        assert response.status_code == SUCCESS_STATUS_CODE
        assert data[0]['experiment_name'] == experiment_name
-        assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['112101'][
-                   'message'] == 'Cost Recommendations Available'
+        assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['111000'][


Can the codes be replaced with the variables defined in utils.py

chandrams · 2023-10-30T12:37:43Z

design/MonitoringModeAPI.md

              "notifications": {
-                "112101": {
+                "111000": {


I don't see notification codes doc with this code and a few other codes are missing, can you please check and update the doc.

I think the doc in mvp_demo is updated, we will need to update the doc in the remote_monitoring branch

chandrams · 2023-10-30T12:44:25Z

tests/scripts/remote_monitoring_tests/helpers/utils.py

@@ -68,7 +68,9 @@
 CRITICAL_MEMORY_LIMIT_NOT_SET_CODE = "524002"

 INFO_COST_RECOMMENDATIONS_AVAILABLE_CODE = "112101"
+INFO_PERFORMANCE_RECOMMENDATIONS_AVAILABLE_CODE = "112102"


I don't see this being used anywhere. Please include tests for validating the notification codes.

Signed-off-by: saakhan <[email protected]>

… term Signed-off-by: saakhan <[email protected]>

Signed-off-by: saakhan <[email protected]>

chandrams · 2023-11-03T11:54:27Z

tests/scripts/remote_monitoring_tests/rest_apis/test_list_recommendations.py

    """
-    Test Description: This test validates list recommendations for multiple experiments posted using different json files
-                      and query with only the parameter latest and with both latest=true and latest=false
+        Test Description: This test validates medium term list recommendations for multiple experiments posted using different json files


Please update the description as this tests all terms

chandrams · 2023-11-03T11:55:05Z

tests/scripts/remote_monitoring_tests/rest_apis/test_list_recommendations.py

@@ -598,8 +606,8 @@ def test_list_recommendations_exp_name_and_monitoring_end_time(test_name, monito
    data = response.json()
    assert response.status_code == SUCCESS_STATUS_CODE
    assert data[0]['experiment_name'] == experiment_name
-    assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['112101'][
-               'message'] == 'Cost Recommendations Available'
+    assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['111000'][


Use variable instead of notification code

chandrams · 2023-11-03T11:55:19Z

tests/scripts/remote_monitoring_tests/rest_apis/test_list_recommendations.py

@@ -448,8 +456,8 @@ def test_list_recommendations_exp_name_and_latest(latest, cluster_type):
    data = response.json()
    assert response.status_code == SUCCESS_STATUS_CODE
    assert data[0]['experiment_name'] == experiment_name
-    assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['112101'][
-               'message'] == 'Cost Recommendations Available'
+    assert data[0]['kubernetes_objects'][0]['containers'][0]['recommendations']['notifications']['111000'][


Use variable instead of notification code

chandrams · 2023-11-03T11:56:59Z

tests/scripts/remote_monitoring_tests/helpers/utils.py

+                        for engine_entry in engines_list:
+                            if engine_entry in terms_obj[term]["recommendation_engines"]:
+                                engine_obj = terms_obj[term]["recommendation_engines"][engine_entry]
+                                validate_config(engine_obj["config"])


As discussed, include validate_variations here

Merge 0.0.20_rm changes from mvp_demo to master

…per review Signed-off-by: saakhan <[email protected]>

chandrams · 2023-11-03T15:59:49Z

tests/scripts/remote_monitoring_tests/helpers/utils.py

-                                    engine_obj = terms_obj[term]["recommendation_engines"][engine_entry]
-                                    validate_config(engine_obj["config"])
+                current_config = list_reco_container["recommendations"]["data"][interval_end_time]["current"]
+


@khansaad - For loop to verify all the terms is required as the long term test would have all 3 terms and medium term will have short term as well and it needs to be validated

…ase and other review changes Signed-off-by: saakhan <[email protected]>

chandrams · 2023-11-07T04:02:04Z

tests/scripts/remote_monitoring_tests/rest_apis/test_list_recommendations.py

    experiment_name = None
    response = list_recommendations(experiment_name, latest)

    list_reco_json = response.json()
    assert response.status_code == SUCCESS_200_STATUS_CODE

    # Validate the json against the json schema
-    errorMsg = validate_list_reco_json(list_reco_json, list_reco_json_schema)
+    errorMsg = validate_list_reco_json(list_reco_json, medium_term_list_reco_json_schema)


This should be reco_json_schema. Have you tested the changes?

Updated.
Also, Ran the complete functional tests, didn't find any issues in that.

Signed-off-by: saakhan <[email protected]>

chandrams

LGTM

basic changes for fixing the sanity failures

76683c8

Signed-off-by: saakhan <[email protected]>

khansaad added the test label Oct 12, 2023

khansaad added this to the Kruize 0.0.20_rm Release milestone Oct 12, 2023

khansaad self-assigned this Oct 12, 2023

khansaad linked an issue Oct 12, 2023 that may be closed by this pull request

Testing Performance Profile changes #978

Closed

khansaad added 2 commits October 12, 2023 16:37

test updates

620dfa1

Signed-off-by: saakhan <[email protected]>

add schema, update docs and other notifications test changes

bffcbc3

Signed-off-by: saakhan <[email protected]>

khansaad changed the title ~~WIP: Add and update tests for the listRecommendations and UpdateRecommendations APIs~~ Add and update tests for the listRecommendations and UpdateRecommendations APIs Oct 26, 2023

khansaad marked this pull request as ready for review October 26, 2023 11:33

khansaad requested a review from chandrams October 30, 2023 07:07

chandrams reviewed Oct 30, 2023

View reviewed changes

khansaad added 3 commits October 31, 2023 18:37

updates wrt the review

f4d54f1

Signed-off-by: saakhan <[email protected]>

update tests for list recommendations, add schema for medium and long…

f283324

… term Signed-off-by: saakhan <[email protected]>

optimize term recommendations test functions, fix e2e tests

4eb2e15

Signed-off-by: saakhan <[email protected]>

chandrams reviewed Nov 3, 2023

View reviewed changes

dinogun and others added 2 commits November 3, 2023 18:40

Merge pull request kruize#1021 from kruize/mvp_demo

032dce5

Merge 0.0.20_rm changes from mvp_demo to master

minor changes to include variation values check, cosmetic changes as …

fd75ef1

…per review Signed-off-by: saakhan <[email protected]>

khansaad requested review from chandrams and dinogun November 3, 2023 13:32

chandrams reviewed Nov 3, 2023

View reviewed changes

khansaad added 3 commits November 6, 2023 11:43

Merge remote-tracking branch 'upstream/master' into perf-profile-tests

77a2167

Merge remote-tracking branch 'upstream/mvp_demo' into perf-profile-tests

194a562

update notifications codes test, add new test function for negative c…

056a8c9

…ase and other review changes Signed-off-by: saakhan <[email protected]>

khansaad requested a review from chandrams November 6, 2023 14:27

chandrams reviewed Nov 7, 2023

View reviewed changes

minor update to change the function parameter name

820bc90

Signed-off-by: saakhan <[email protected]>

chandrams approved these changes Nov 7, 2023

View reviewed changes

dinogun merged commit 27e24ae into kruize:mvp_demo Nov 7, 2023
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and update tests for the listRecommendations and UpdateRecommendations APIs #981

Add and update tests for the listRecommendations and UpdateRecommendations APIs #981

khansaad commented Oct 12, 2023

chandrams Oct 30, 2023

chandrams Oct 30, 2023

chandrams Oct 30, 2023

chandrams Oct 30, 2023

chandrams Oct 30, 2023

chandrams Oct 30, 2023

chandrams Nov 3, 2023

chandrams Nov 3, 2023

chandrams Nov 3, 2023

chandrams Nov 3, 2023

chandrams Nov 3, 2023

chandrams Nov 7, 2023

khansaad Nov 7, 2023

chandrams left a comment

Add and update tests for the listRecommendations and UpdateRecommendations APIs #981

Add and update tests for the listRecommendations and UpdateRecommendations APIs #981

Conversation

khansaad commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandrams left a comment

Choose a reason for hiding this comment