-
Notifications
You must be signed in to change notification settings - Fork 5
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
Tests/formatting #105
Tests/formatting #105
Conversation
There are too many exceptions for me to add to SonarQube whitelist, would you mind swapping the address literals to constants, for example something like: IPV6_ADDRESSES = [
'2606:4700:4700::1111',
...
]
TEST_CASES_IPV6 = [
IPv6(),
IPv6(IPV6_ADDRESSES[0]),
IPv6(IPV6_ADDRESSES[0], 80),
IPv6(f'[{IPV6_ADDRESSES[0]}]:80'),
IPv6(f'[{IPV6_ADDRESSES[0]}]:80', 8080),
] That way I wouldn't need to manually approve a hundred different ones. |
Oh yeah, apparently I've forgotten to replace |
Yes, no problem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my changes and merged them to your branch to make your life a little easier.
Signed-off-by: Lari Liuhamo <lari.liuhamo@gmail.com>
I tried to do as you suggested and it is not working... Can you add the exception manually? Or can I do that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good, just gotta do the same for the other two files now
I'll take a better look after work |
It was nothing complicated, the test result simply included the quotes of the string literal. An easy fix was to make Python treat the IP address via |
can we not just add the the exception? The code will be much cleaner then.... |
Okay, fine, I'll spend the weekend telling Sonarcloud to ignore the false positives. You can revert the changes to the addresses. |
Actually, I saw that you did already a lot in the code. Let's change the code ;-) I will do the rest this week sorry for the misunderstanding |
Here we are :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Not sure why the unit tests for the pull request aren't running, looking into it. |
Ah, whatever. It already passes the tests anyway, so I'll force-merge this. Dunno why it's not running in the pull request itself. EDIT: This is kinda off-topic, but as you might've observed I'm starting to require signed commits on the main branches of some of my projects. I'm not going to be super strict about it, I'm going to merge this anyway, but you might want to look into GPG signing Git commits automatically. You can see my commits are verified with my own GPG signature, so you know they're all made by me and not someone impersonating me. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
At #105 and https://github.com/orgs/community/discussions/26698#discussioncomment-7094600 it was reported, that the unit tests were not triggered on a pull request. The reason was that the trigger for a pull request was not active. The trigger `pull_request` is used for PRs - see https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request for details Note that the workflow https://github.com/Diapolo10/iplib3/blob/main/.github/workflows/codeql-analysis.yml has the correct triggers. I also added `workflow_dispatch` to enable manual triggering.
Hey,
now all the tests are parametrised.
This is quite useful because it makes easy to quickly add more tests.
In addition, pytest generates exactly one test for each test case. This way you can see exactly which test case is causing the test to fail. That is very useful in case of nasty bugs ;-)