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

Tests/formatting #105

Merged
merged 26 commits into from
Sep 25, 2023
Merged

Tests/formatting #105

merged 26 commits into from
Sep 25, 2023

Conversation

VascoSch92
Copy link
Contributor

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 ;-)

@Diapolo10
Copy link
Owner

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.

@Diapolo10
Copy link
Owner

Oh yeah, apparently I've forgotten to replace pylint and flake8 in this project with Ruff. I should probably get to it.

@Diapolo10 Diapolo10 self-requested a review September 18, 2023 14:57
@VascoSch92
Copy link
Contributor Author

VascoSch92 commented Sep 18, 2023

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.

Yes, no problem

Copy link
Owner

@Diapolo10 Diapolo10 left a 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.

@Diapolo10 Diapolo10 added enhancement New feature or request good first issue Good for newcomers test Improvements to automated testing python Pull requests that update Python code labels Sep 18, 2023
@VascoSch92
Copy link
Contributor Author

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.

Yes, no problem

I tried to do as you suggested and it is not working... Can you add the exception manually? Or can I do that?

Copy link
Owner

@Diapolo10 Diapolo10 left a 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

@Diapolo10
Copy link
Owner

I'll take a better look after work

@Diapolo10
Copy link
Owner

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 repr instead of str in the test case.

@VascoSch92
Copy link
Contributor Author

can we not just add the the exception? The code will be much cleaner then....

@Diapolo10
Copy link
Owner

Okay, fine, I'll spend the weekend telling Sonarcloud to ignore the false positives. You can revert the changes to the addresses.

@VascoSch92
Copy link
Contributor Author

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

@VascoSch92
Copy link
Contributor Author

Here we are :-)

Copy link
Owner

@Diapolo10 Diapolo10 left a comment

Choose a reason for hiding this comment

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

LGTM

@Diapolo10
Copy link
Owner

Not sure why the unit tests for the pull request aren't running, looking into it.

@Diapolo10
Copy link
Owner

Diapolo10 commented Sep 25, 2023

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.

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Diapolo10 Diapolo10 merged commit 5f33c55 into Diapolo10:main Sep 25, 2023
6 checks passed
@koppor koppor mentioned this pull request Sep 29, 2023
Diapolo10 added a commit that referenced this pull request Sep 29, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers python Pull requests that update Python code test Improvements to automated testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants