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

[GT-157] Drop messages from banned dns, and add tests for messages saving to the correct queue #238

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

rowan04
Copy link
Contributor

@rowan04 rowan04 commented Jun 19, 2023

  • compares message signer with dns in banned dns list
  • which is got from config file and sorted through

resolves #184

- compares message signer with dns in banned dns list
- which is got from config file and sorted through
ssm/ssm2.py Fixed Show resolved Hide resolved
- now makes more sense
- has an error message
- reworking of some if/elif/else statements
ssm/ssm2.py Fixed Show resolved Hide resolved
@rowan04
Copy link
Contributor Author

rowan04 commented Jun 21, 2023

unit tests to be written

@rowan04 rowan04 marked this pull request as ready for review June 21, 2023 11:48
@rowan04 rowan04 requested a review from a team as a code owner June 21, 2023 11:48
@tofu-rocketry tofu-rocketry marked this pull request as draft June 26, 2023 15:10
@rowan04
Copy link
Contributor Author

rowan04 commented Jul 20, 2023

Pushed what I was working on as going on holiday

@rowan04
Copy link
Contributor Author

rowan04 commented Jul 20, 2023

Currently errors as below:

image

test/test_ssm.py Fixed Show resolved Hide resolved
test/test_ssm.py Fixed Show resolved Hide resolved
@tofu-rocketry tofu-rocketry self-assigned this Aug 1, 2023
@rowan04 rowan04 changed the title Drop messages from banned dns [GT-157] Drop messages from banned dns Aug 16, 2023
- Mock out the _handle_msg() function called by _save_msg_to_queue() (the function we are testing), as it fails due to client signers and certificates not matching.
- It is easier to mock out the failing function instead, as it is not the function we are testing in this test.
- To test _save_msg_to_queue, we are assuming the message's certificate and signer match.
ssm/ssm2.py Show resolved Hide resolved
test/test_ssm.py Fixed Show fixed Hide fixed
test/test_ssm.py Fixed Show fixed Hide fixed
test/test_ssm.py Fixed Show fixed Hide fixed
test/test_ssm.py Fixed Show resolved Hide resolved
test/test_ssm.py Fixed Show fixed Hide fixed
- this was outdated code no longer in use
@rowan04
Copy link
Contributor Author

rowan04 commented Sep 19, 2023

image
this is the current output when you run the test... is this fine? Or would you like more assert tests or something?

@rowan04 rowan04 marked this pull request as ready for review September 19, 2023 16:24
- we can check the log output matches an expected string
- to check the test has passed or failed
@rowan04
Copy link
Contributor Author

rowan04 commented Sep 20, 2023

this is the current output when you run the test... is this fine? Or would you like more assert tests or something?

This has been improved with the addition of assertion tests using LogCapture. New output:
image

@rowan04
Copy link
Contributor Author

rowan04 commented Sep 20, 2023

A failing test:
image

- also re-adds the creation of the certificate which was missing
@rowan04
Copy link
Contributor Author

rowan04 commented Sep 20, 2023

New output with inclusion of the rejection queue tests and re-addding of making the certificate (which was causing a failure)
image

@rowan04 rowan04 changed the title [GT-157] Drop messages from banned dns [GT-157] Drop messages from banned dns, and add tests for messages saving to the correct queue Sep 20, 2023
ssm/agents.py Show resolved Hide resolved
ssm/agents.py Outdated Show resolved Hide resolved
ssm/agents.py Outdated Show resolved Hide resolved
test/test_ssm.py Outdated
for dn in valid_dns:
# Capture the log output so we can use it in assertions
with LogCapture() as log:
print("Testing dn:", dn)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't generally be printing in tests. We only care about the output if something fails.

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

- removes printing in test
- rewrites line using with context manager
- changes variable name
- passes cp as an argument to the fucntion
- shortens long lines
@rowan04
Copy link
Contributor Author

rowan04 commented Sep 27, 2023

new test output. do you want the log output printing removed as well?
image

@rowan04
Copy link
Contributor Author

rowan04 commented Sep 27, 2023

updating to latest dev

ssm/agents.py Outdated
else:
log.warning('DN in banned dns list is not in '
'the correct format: %s', line)
finally:
Copy link
Member

Choose a reason for hiding this comment

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

Now we have a context manager for the file, we can get rid of the try...finally wrapper (and the `f=None') as the file will close as soon as the handler is exited (whether normally or by exception).

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

test/test_ssm.py Outdated

ssm._save_msg_to_queue(message_rejected, empaid)

print(str(log))
Copy link
Member

Choose a reason for hiding this comment

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

There's this and the print below that also need removing.

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

@tofu-rocketry
Copy link
Member

testfixtures will need adding to requirements-test.txt so it's available in Travis.

- this makes it available in Travis
- used in class TestMsgToQueue
- removes remaining print statements to simplify logging output
- adds teardown
- also improves comments
- with the addition of a context manager for a file in an earlier commit, this wasn't needed
test/test_ssm.py Dismissed Show dismissed Hide dismissed
@tofu-rocketry tofu-rocketry added this to the 3.5.0 milestone Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

GT-157 Add option to completely drop messages from banned DNs
2 participants