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

add more valid/invalid cases in ipv4 action's tests #848

Open
wants to merge 3 commits into
base: v1-beta
Choose a base branch
from

Conversation

EltonLobo07
Copy link
Contributor

No description provided.

Copy link

pkg-pr-new bot commented Sep 24, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/valibot@848

commit: cd70108

@EltonLobo07
Copy link
Contributor Author

Do you think I have covered the majority of the cases? If not, can you help me understand what you mean by cover all possible scenarios based on the regex used?

@fabian-hiller fabian-hiller self-assigned this Sep 25, 2024
@fabian-hiller fabian-hiller added the enhancement New feature or request label Sep 25, 2024
@fabian-hiller
Copy link
Owner

fabian-hiller commented Sep 25, 2024

This is our IP v4 regex:

/^(?:(?:[1-9]|1\d|2[0-4])?\d|25[0-5])(?:\.(?:(?:[1-9]|1\d|2[0-4])?\d|25[0-5])){3}$/u

The idea is basically to test the entire regex definition, so if we accidentally change the regex in the future, our tests should fail and warn us.

For example, if a regex contains the pattern [3-6], we should have a positive test for 3, 6, and optionally a value in between, and a negative test for 2 and 7.

@fabian-hiller
Copy link
Owner

Thank you very much! I am busy this weekend, but I will try to review it next week.

@EltonLobo07
Copy link
Contributor Author

I think I can improve the added test cases. I'll let you know once I am done (probably tomorrow) so that you can review the changes then.

@EltonLobo07
Copy link
Contributor Author

Updated. Feel free to review the PR.

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

Successfully merging this pull request may close these issues.

2 participants