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

RPKI parse tests #49

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

RPKI parse tests #49

wants to merge 4 commits into from

Conversation

jurraca
Copy link
Contributor

@jurraca jurraca commented Jan 18, 2025

Add initial tests for RPKI parsing: check that duplicates, incompletes, ROA type, and invalids are parsed correctly. Mostly, setting up a test Context and adjusting some setup.

Includes a RPKI fixture CSV and handles casting it to a rpki_raw.json file as expected by the parsing code.

rebased on #47

the previously added check for leading zeros picks up private networks,
so it's not exactly what we want here. Instead, separate the format_pfx
and is_valid_pfx intents and add tests accordingly.
format_pfx checks whether the existing prefix can be coerced to a string
and returns it. is_valid_pfx simply checks whether it's a valid pfx
or not.
set up a test context to use with pytest's tmp_path
pass in the epoch argument exlicitly to prevent conflicts,
and adjust context argument parsing to handle that.
Add a rpki_raw.json fixture, and a tests/util.py module.
Copy link
Collaborator

@fjahr fjahr left a comment

Choose a reason for hiding this comment

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

Took a first look but will revisit once #47 is merged.

@@ -39,6 +37,12 @@ def __init__(self, args):
self.epoch = self.args.epoch
self.epoch_dir = "r" + self.args.epoch
self.epoch_datetime = repro_epoch
# This is only for testing, to set an epoch in the Test context.
# the CLI parser prevents this argument from existing without the 'reproduce' arg
elif self.args.epoch:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, couldn't we do this from outside of the context by mocking the epoch? Unless it's much more complex I would prefer not to add test specific code in the normal code.

tests/context.py Outdated
Comment on lines 40 to 41
TEST_ARGS.epoch = epoch
context = Context(TEST_ARGS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding to what I mentioned above, didn't test it though.

Suggested change
TEST_ARGS.epoch = epoch
context = Context(TEST_ARGS)
context = Context(TEST_ARGS)
context.epoch = epoch
context.epoch_dir = epoch
context.epoch_datetime = datetime.utcfromtimestamp(int(epoch))

@@ -48,7 +48,10 @@ def __init__(self, args):
self.epoch_dir = str(int(utc_time_now))
self.epoch_datetime = datetime.utcfromtimestamp(int(utc_time_now))

cwd = os.getcwd()
if hasattr(self.args, 'cwd'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I don't see what this is needed for.

@fjahr
Copy link
Collaborator

fjahr commented Jan 19, 2025

Needs rebase now that #47 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants