Add general_merge tests and a test data generator module #32

jurraca · 2024-11-04T17:34:04Z

Initial approach to tests, starting with the merge process. The generate_data module has functions to create IP networks and addrs and create AS files as we would get from RPKI or RIR.

fjahr

Added a few comments. I think adding such a test with generated data is always interesting but it's not where I would have started :) So something I would still like to have is very basic test vectors with static data, primarily because these are very easy to reason about and after we have set it up once it becomes trivial to add all kinds of edge cases we will stumble over. We could have them in CSV file like this one from BIP340: https://github.com/bitcoin/bips/blob/master/bip-0340/test-vectors.csv But let's add this here first since it's already in pretty good shape.

fjahr · 2024-11-12T14:44:51Z

tests/generate_data.py

+from random import randint
+
+
+def generate_ip(ip_type="v4", subnet_size="16"):


nit: The generate_data.py file could be moved into a util or shared folder. But can be done as a follow-up when more tests are added.

fjahr · 2024-11-12T15:03:20Z

tests/generate_data.py

+def generate_asns(count):
+    asns = []
+    while count > 0:
+        asn = randint(1, 100000)


Let's use our own max asn value for the end of the range here, i.e. max_encode. Can also use the default for now since we are pretty married to it currently with the assumptions in the core code: 33521664.

fjahr · 2024-11-12T15:04:10Z

tests/generate_data.py

+    return network
+
+
+def generate_ip_networks(


Hm, the ip networks (and to some degree asns too) generate methods don't have guarantees that the generated result is a set without overlap, right? While the chance seems low, there could be overlapping results which means the test outcomes are not deterministic. I think it would be neat if these functions could always generate a set. This would then allow us to make stricter testing. I.e. we could be sure that merging two sets of 10 with overlap of 1 have a result of 19 entries.

yes, done. However it's a bit more complicated as comparing sets of networks requires checking whether the networks are subnets of each other, hence the make_disjoint function to enforce non-overlapping networks after generation.

fjahr · 2024-11-12T15:05:40Z

tests/generate_data.py

+    """
+    extra_new = []
+    for extra in extra_items:
+        included = 0


nit: Could use boolean here

fjahr · 2024-11-12T15:10:42Z

tests/generate_data.py

+        end_ip = int(ipaddress.IPv4Address("255.255.255.255"))
+        subnet_mask = end_ip << (32 - subnet_size)


I think this is cleaner:

subnet_mask = ipaddress.IPv4Network(f"0.0.0.0/{subnet_size}", strict=False).netmask

fjahr · 2024-11-12T15:15:07Z

tests/merge_test.py

+
+TEST_FILE_PATHS = {


I think the files are not cleaned up after each test run yet, right? should do that to avoid test polution.

thanks -- I went with using pytest's tmp_path which writes to tmp and overwrites on every run, but doesn't clean up the run after it's finished.

fjahr · 2024-11-24T22:13:42Z

Changes look good so far but similar to what I reported in the other PR, I am not sure how to run the test currently:

 [nix] pytest tests/merge_test.py
===================================================================================================================== test session starts =====================================================================================================================
platform darwin -- Python 3.11.9, pytest-8.1.1, pluggy-1.4.0
rootdir: /Users/XXX/projects/python/kartograf
collected 0 items / 1 error

=========================================================================================================================== ERRORS ============================================================================================================================
____________________________________________________________________________________________________________ ERROR collecting tests/merge_test.py _____________________________________________________________________________________________________________
ImportError while importing test module '/Users/XXX/projects/python/kartograf/tests/merge_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/nix/store/na2a6spzlqg0rrgz8462bla7hxvygjcq-python3-3.11.9/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/merge_test.py:10: in <module>
    from kartograf.merge import general_merge
E   ModuleNotFoundError: No module named 'kartograf'

jurraca · 2024-11-25T14:55:54Z

hmm sorry, not sure what happened there. I should have stacked these test PRs.

fjahr · 2024-11-26T11:58:50Z

Now the test works but I am getting these deprecation warnings. Maybe another version incompatibility issue?

$ pytest tests
===================================================================================================================== test session starts =====================================================================================================================
platform darwin -- Python 3.12.6, pytest-8.3.3, pluggy-1.5.0
rootdir: /Users/XXX/projects/python/kartograf
collected 3 items

tests/merge_test.py ...                                                                                                                                                                                                                                 [100%]

====================================================================================================================== warnings summary =======================================================================================================================
tests/merge_test.py::test_merge
tests/merge_test.py::test_merge_disjoint
tests/merge_test.py::test_merge_joint
  /Users/XXX/projects/python/kartograf/venv/lib/python3.12/site-packages/pandas/core/dtypes/cast.py:1641: DeprecationWarning: np.find_common_type is deprecated.  Please use `np.result_type` or `np.promote_types`.
  See https://numpy.org/devdocs/release/1.25.0-notes.html and the docs for more information.  (Deprecated NumPy 1.25)
    return np.find_common_type(types, [])

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================================================================ 3 passed, 3 warnings in 0.70s ================================================================================================================

jurraca · 2024-11-27T19:12:33Z

iiuc Pandas should support numpy > 1.x , so this is odd. I can't see anything in pandas about this deprecation in particular. Our pandas version requirement is ==1.5.3 which is a bit old (we could at least have it be >=1.5.3). That deprecation went into effect in July 2023. I've been running pandas 2.2.1 with numpy 1.26.1 so haven't seen it.

what pandas version do you have?

fjahr · 2024-12-07T19:39:48Z

what pandas version do you have?

Sorry, I thought I had already answered here. I did have 1.5.3. It looks good to me now when I use the new pandas version but it seems this branch is conflicting with the upgrade of the pandas version in master. Can you rebase one more time, then it should be ready to go :)

the generate_data module has functions to create IP networks and addrs and create AS files as we would get from RPKI or RIR.

fjahr

ACK

fjahr · 2024-12-11T15:19:53Z

tests/merge_test.py

+    main_data = generate_file_items(100)
+    main_ips = [item.split()[0] for item in main_data]
+    rpki_ips = main_ips[:50]
+    irr_ips = main_ips[50:]


Just a note (also to myself), since we may remove IRR as a source in the short-mid term when RPKI has adopted broadly enough, we shouldn't hardcode it in the tests unless necessary. Here I think the variables could have been named base and extra as well and that's a bit more future-proof.

fjahr reviewed Nov 12, 2024

View reviewed changes

jurraca force-pushed the merge-tests branch 2 times, most recently from 65c42c1 to 2f9991d Compare November 18, 2024 11:53

jurraca mentioned this pull request Nov 18, 2024

Add test vectors jurraca/kartograf#1

Open

jurraca force-pushed the merge-tests branch from 2f9991d to f043983 Compare November 18, 2024 12:04

jurraca mentioned this pull request Nov 18, 2024

Test vectors #37

Merged

jurraca force-pushed the merge-tests branch 3 times, most recently from 9899f4b to 866834f Compare November 25, 2024 14:55

jurraca force-pushed the merge-tests branch from 866834f to 92a038d Compare November 28, 2024 15:58

jurraca mentioned this pull request Dec 3, 2024

bump pandas requirement to >=2.2.3 #41

Merged

Add general_merge tests and a test data generator module

3866ecc

the generate_data module has functions to create IP networks and addrs and create AS files as we would get from RPKI or RIR.

jurraca force-pushed the merge-tests branch from 92a038d to 3866ecc Compare December 9, 2024 17:24

fjahr reviewed Dec 11, 2024

View reviewed changes

fjahr merged commit b5dda3b into asmap:master Dec 11, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add general_merge tests and a test data generator module #32

Add general_merge tests and a test data generator module #32

jurraca commented Nov 4, 2024

fjahr left a comment

fjahr Nov 12, 2024

fjahr Nov 12, 2024

fjahr Nov 12, 2024

jurraca Nov 18, 2024

fjahr Nov 12, 2024

fjahr Nov 12, 2024

fjahr Nov 12, 2024

jurraca Nov 18, 2024

fjahr commented Nov 24, 2024

jurraca commented Nov 25, 2024

fjahr commented Nov 26, 2024 •

edited

Loading

jurraca commented Nov 27, 2024

fjahr commented Dec 7, 2024

fjahr left a comment

fjahr Dec 11, 2024

		from random import randint


		def generate_ip(ip_type="v4", subnet_size="16"):

		end_ip = int(ipaddress.IPv4Address("255.255.255.255"))
		subnet_mask = end_ip << (32 - subnet_size)


		TEST_FILE_PATHS = {

Add general_merge tests and a test data generator module #32

Add general_merge tests and a test data generator module #32

Conversation

jurraca commented Nov 4, 2024

fjahr left a 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

fjahr commented Nov 24, 2024

jurraca commented Nov 25, 2024

fjahr commented Nov 26, 2024 • edited Loading

jurraca commented Nov 27, 2024

fjahr commented Dec 7, 2024

fjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjahr commented Nov 26, 2024 •

edited

Loading