From 634be7223f0f5addc9d5cec856b8d27a30dd9a52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arturo=20Filast=C3=B2?= Date: Wed, 24 Apr 2024 10:02:13 +0200 Subject: [PATCH 1/3] Delete the unused round_robin function and the functional test for it Move the function into the unittest to use it as test --- api/ooniapi/probe_services.py | 24 --------------- api/tests/functional/test_probe_services.py | 33 --------------------- api/tests/unit/test_probe_services.py | 33 +++++++++++++++++---- 3 files changed, 27 insertions(+), 63 deletions(-) delete mode 100644 api/tests/functional/test_probe_services.py diff --git a/api/ooniapi/probe_services.py b/api/ooniapi/probe_services.py index 77c10dc9d..4a0c42f5e 100644 --- a/api/ooniapi/probe_services.py +++ b/api/ooniapi/probe_services.py @@ -511,30 +511,6 @@ def random_web_test_helpers(th_list: List[str]) -> List[Dict]: return out -def round_robin_web_test_helpers() -> List[Dict]: - """Round robin test helpers based on the probe ipaddr. - 0.th is special and gets only 10% of the traffic. - """ - try: - ipa = extract_probe_ipaddr() - # ipaddr as (large) integer representation (v4 or v6) - q = int(ipaddress.ip_address(ipa)) - q = q % 100 - except Exception: - q = 12 # pick 1.th - - if q < 10: - shift = 0 - else: - shift = q % 4 + 1 - - out = [] - for n in range(5): - n = (n + shift) % 5 - out.append({"address": f"https://{n}.th.ooni.org", "type": "https"}) - - return out - def generate_test_helpers_conf() -> Dict: # Load-balance test helpers deterministically diff --git a/api/tests/functional/test_probe_services.py b/api/tests/functional/test_probe_services.py deleted file mode 100644 index dc560339c..000000000 --- a/api/tests/functional/test_probe_services.py +++ /dev/null @@ -1,33 +0,0 @@ -from collections import Counter -import ooniapi.probe_services - -from unittest.mock import patch - - -@patch("ooniapi.probe_services.extract_probe_ipaddr") -def test_round_robin_web_test_helpers(mock): - # test fallback without mocked ipaddr - li = ooniapi.probe_services.round_robin_web_test_helpers() - assert li == [ - {"address": "https://1.th.ooni.org", "type": "https"}, - {"address": "https://2.th.ooni.org", "type": "https"}, - {"address": "https://3.th.ooni.org", "type": "https"}, - {"address": "https://0.th.ooni.org", "type": "https"}, - ] - - c = Counter() - for n in range(200): - mock.return_value = f"1.2.3.{n}" - li = ooniapi.probe_services.round_robin_web_test_helpers() - assert len(li) == 4 - addr = li[0]["address"] # statistics on the first TH returned - c[addr] += 1 - - assert c == Counter( - { - "https://0.th.ooni.org": 20, - "https://1.th.ooni.org": 60, - "https://2.th.ooni.org": 60, - "https://3.th.ooni.org": 60, - } - ) diff --git a/api/tests/unit/test_probe_services.py b/api/tests/unit/test_probe_services.py index 96d1f4756..74480f2ba 100644 --- a/api/tests/unit/test_probe_services.py +++ b/api/tests/unit/test_probe_services.py @@ -1,12 +1,33 @@ -from unittest.mock import patch +import ipaddress import ooniapi.probe_services -from ooniapi.probe_services import random_web_test_helpers, round_robin_web_test_helpers +from ooniapi.probe_services import random_web_test_helpers +def round_robin_web_test_helpers(ipa) -> List[Dict]: + """Round robin test helpers based on the probe ipaddr. + 0.th is special and gets only 10% of the traffic. + """ + try: + # ipaddr as (large) integer representation (v4 or v6) + q = int(ipaddress.ip_address(ipa)) + q = q % 100 + except Exception: + q = 12 # pick 1.th -@patch("ooniapi.probe_services.extract_probe_ipaddr") -def test_web_test_helpers(mock): - mock.return_value = "1.2.3.4" + if q < 10: + shift = 0 + else: + shift = q % 4 + 1 + + out = [] + for n in range(5): + n = (n + shift) % 5 + out.append({"address": f"https://{n}.th.ooni.org", "type": "https"}) + + return out + + +def test_web_test_helpers(): r1 = random_web_test_helpers( [ "https://0.th.ooni.org", @@ -16,7 +37,7 @@ def test_web_test_helpers(mock): "https://4.th.ooni.org", ] ) - r2 = round_robin_web_test_helpers() + r2 = round_robin_web_test_helpers("1.2.3.4") th1 = set(list(map(lambda x: x["address"], r1))) th2 = set(list(map(lambda x: x["address"], r2))) assert th1 == th2 From 591c343acb2763bcca40b3a2c4ca514023cca2a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arturo=20Filast=C3=B2?= Date: Wed, 24 Apr 2024 10:03:03 +0200 Subject: [PATCH 2/3] Do not hardcode analysis outcomes in integration tests Checking for hardcoded int values for analysis is very brittle, since the analysis might change (as it has) and all the numbers need to be manually updated. Instead we check for reasonable ranges of values that are less likely to change in the future. --- api/tests/integ/test_aggregation.py | 80 ++++++++++++----------------- 1 file changed, 32 insertions(+), 48 deletions(-) diff --git a/api/tests/integ/test_aggregation.py b/api/tests/integ/test_aggregation.py index dda68475d..13443400d 100644 --- a/api/tests/integ/test_aggregation.py +++ b/api/tests/integ/test_aggregation.py @@ -185,17 +185,14 @@ def test_aggregation_no_axis_filter_multi_probe_cc(client): url = "aggregation?probe_cc=BR,GB&since=2021-07-09&until=2021-07-10" r = api(client, url) r.pop("db_stats", None) - assert r == { - "dimension_count": 0, - "result": { - "anomaly_count": 123, - "confirmed_count": 0, - "failure_count": 113, - "measurement_count": 2435, - "ok_count": 2199, - }, - "v": 0, - }, fjd(r) + + assert r["dimension_count"] == 0 + assert r["v"] == 0 + assert r["result"]["anomaly_count"] > 0 + assert r["result"]["measurement_count"] > 0 + assert r["result"]["ok_count"] > 0 + assert isinstance(r["result"]["failure_count"], int) + assert isinstance(r["result"]["confirmed_count"], int) def test_aggregation_no_axis_filter_multi_test_name(client): @@ -203,17 +200,14 @@ def test_aggregation_no_axis_filter_multi_test_name(client): url = "aggregation?test_name=web_connectivity,whatsapp&since=2021-07-09&until=2021-07-10" r = api(client, url) r.pop("db_stats", None) - assert r == { - "dimension_count": 0, - "result": { - "anomaly_count": 319, - "confirmed_count": 42, - "failure_count": 340, - "measurement_count": 8547, - "ok_count": 7846, - }, - "v": 0, - }, fjd(r) + + assert r["dimension_count"] == 0 + assert r["v"] == 0 + assert r["result"]["anomaly_count"] > 0 + assert r["result"]["measurement_count"] > 0 + assert r["result"]["ok_count"] > 0 + assert isinstance(r["result"]["failure_count"], int) + assert isinstance(r["result"]["confirmed_count"], int) def test_aggregation_no_axis_filter_multi_test_name_1_axis(client): @@ -313,32 +307,22 @@ def test_aggregation_x_axis_only_invalid_time_grain_too_large(client, log): def test_aggregation_x_axis_only_hour(client, log): # 1 dimension: X url = "aggregation?since=2021-07-09&until=2021-07-11&axis_x=measurement_start_day" - r = api(client, url) - r.pop("db_stats", None) - expected = { - "dimension_count": 1, - "result": [ - { - "anomaly_count": 686, - "confirmed_count": 42, - "failure_count": 777, - "measurement_count": 9990, - "measurement_start_day": "2021-07-09T00:00:00Z", - "ok_count": 8485, - }, - { - "anomaly_count": 0, - "confirmed_count": 0, - "failure_count": 0, - "measurement_count": 1, - "measurement_start_day": "2021-07-09T01:00:00Z", - "ok_count": 1, - }, - ], - "v": 0, - } - assert r == expected, fjd(r) - + resp = api(client, url) + resp.pop("db_stats", None) + assert resp["dimension_count"] == 1 + assert len(resp["result"]) == 2 + + total_msmts = 0 + for r in resp["result"]: + total_msmts += r["result"]["measurement_count"] + assert r["dimension_count"] == 0 + assert r["v"] == 0 + assert isinstance(r["result"]["anomaly_count"], int) + assert isinstance(r["result"]["measurement_count"], int) + assert isinstance(r["result"]["ok_count"], int) + assert isinstance(r["result"]["failure_count"], int) + assert isinstance(r["result"]["confirmed_count"], int) + assert total_msmts > 0 def test_aggregation_x_axis_domain(client, log): # 1 dimension: X From fe436203efd9eaae9e870bfd79411aca98d5d4ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arturo=20Filast=C3=B2?= Date: Wed, 24 Apr 2024 10:06:50 +0200 Subject: [PATCH 3/3] Increase sleep time to avoid race --- ooniapi/services/oonirun/tests/test_oonirun.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ooniapi/services/oonirun/tests/test_oonirun.py b/ooniapi/services/oonirun/tests/test_oonirun.py index 737fb4ccb..83ddeaaf8 100644 --- a/ooniapi/services/oonirun/tests/test_oonirun.py +++ b/ooniapi/services/oonirun/tests/test_oonirun.py @@ -493,7 +493,7 @@ def test_oonirun_revisions(client, client_with_user_role): j = r.json() first_date_created = j["date_created"] - time.sleep(1) + time.sleep(1.2) j["nettests"][0]["inputs"].append("https://foo2.net/") r = client_with_user_role.put( f"/api/v2/oonirun/links/{oonirun_link_id_one}", json=j