Skip to content

[Validator] Add tests for MacAddress #60422

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

Open
wants to merge 6 commits into
base: 7.3
Choose a base branch
from
Open

[Validator] Add tests for MacAddress #60422

wants to merge 6 commits into from

Conversation

tcoch
Copy link

@tcoch tcoch commented May 14, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

This PR aims to add some tests to the MacAddress Validator.

[x] Add testing for empty string and null values Already handled with testNullIsValid and testEmptyStringIsValid methods
[x] Add testing of unvalid mac addresses
[x] Create generic method assertViolation() in ConstraintValidatorTestCase.php to test that unvalid values do throw a violation. See conversation below

If this PR is accepted, I'll try to continue this work on other validators. I believe testing empty string and null values is important to ensure no BC occurs, for example.

@carsonbot carsonbot added this to the 7.3 milestone May 14, 2025
@carsonbot carsonbot changed the title [VALIDATOR] Add tests for MAC Address [Validator] Add tests for MAC Address May 14, 2025
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

-1 for adding assertViolation. We should encourage using the existing expectViolationsAt or buildViolation() methods to define precise expected violations instead.

@tcoch tcoch requested a review from stof May 20, 2025 12:50
@OskarStark OskarStark changed the title [Validator] Add tests for MAC Address [Validator] Add tests for MacAddress May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants