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

chore: add some unit test on notification #65

Merged

Conversation

acolombier
Copy link
Contributor

As suggested in the issue #62 , I tasked myself with increasing test coverage and thought I will get started with notifications.

@acolombier
Copy link
Contributor Author

Since this task requires some minimal refactoring in order to split the code in smaller independent sections, I thought I will start with byte threshold first and wait for your feedback before going ahead and doing all of them.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Feb 8, 2023

Sounds good. Just leave me the refactor tasks. The ideal thing is that you exclusively work inside "tests" modules.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Feb 8, 2023

However, if you already started refactoring keep going.
Just let me know when and where you need it next time.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Feb 8, 2023

At a first read it seems a very good start btw!
It's nice you decided to put that function in the notifications struct, feel free to do the same for the other notifications.
Thanks!

@acolombier acolombier force-pushed the chore/unit-tests-bytes-notifcations branch 2 times, most recently from 9ed5861 to 174052a Compare February 9, 2023 10:02
@acolombier acolombier changed the title chore: add some unit test on byte threshold notification chore: add some unit test on notification Feb 9, 2023
@acolombier acolombier force-pushed the chore/unit-tests-bytes-notifcations branch from 174052a to 1caf8b8 Compare February 9, 2023 10:03
@acolombier acolombier marked this pull request as ready for review February 9, 2023 10:03
@GyulyVGC
Copy link
Owner

GyulyVGC commented Feb 9, 2023

Favorites Notifications are ok, but could you leave the management for packets and bytes as it was before?
I mean, in your from functions.

@acolombier
Copy link
Contributor Author

Do you mind pointing out which bits your are referring to? Do you mean the Default trait implementation?

@acolombier acolombier force-pushed the chore/unit-tests-bytes-notifcations branch from 1caf8b8 to 94819da Compare February 10, 2023 10:29
@acolombier acolombier force-pushed the chore/unit-tests-bytes-notifcations branch from 94819da to 8da1436 Compare February 10, 2023 10:38
@acolombier
Copy link
Contributor Author

I have reverted the PacketsNotification and BytesNotification behaviors. I kept the trim addition so there is more edge cases covered, although the UI doesn't let you input anything else.

@GyulyVGC
Copy link
Owner

Thanks! Once also the formatting will be fixed I'll review it! 👍

@GyulyVGC
Copy link
Owner

I just fixed the formatting (note that this can be automatically done with cargo fmt) and reviewed the PR and you did a great job.
It's nice you decided to start from the text input fields which are the most vulnerable part of the app.
Tyvm!

@GyulyVGC GyulyVGC merged commit 1c491a2 into GyulyVGC:main Feb 11, 2023
@GyulyVGC
Copy link
Owner

@all-contributors please add @acolombier for test translation

@allcontributors
Copy link
Contributor

@GyulyVGC

I've put up a pull request to add @acolombier! 🎉

@GyulyVGC GyulyVGC added the enhancement New feature, request, or improvement label Mar 27, 2023
GyulyVGC added a commit that referenced this pull request May 7, 2023
…tions

chore: add some unit test on notification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, request, or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants