NotificationsYou must be signed in to change notification settings
Fork2.2k
Star24.6k

Add validation tests for`_internal/_validators.py`#10763

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

sydney-runkle merged 3 commits intopydantic:mainfromtkasuz:increase_types_test_coverage

Nov 16, 2024

Merged

Add validation tests for`_internal/_validators.py`#10763

sydney-runkle merged 3 commits intopydantic:mainfromtkasuz:increase_types_test_coverage

Nov 16, 2024

Conversation

Copy link

Contributor

tkasuz commentedNov 4, 2024•
edited
Loading

Change Summary

This PR introduces new unit tests to enhance the test coverage by covering the following scenarios:

Added a type check for decimal validators and the corresponding unit tests.
Added unit tests to validate numeric constraints (e.g., 'gt', 'lt', etc.).
Added a unit test to validate theFraction type.

Confirmed that the test coverage for_internal/_validators.py increased from 96.43% to 100%

Related issue number

#7656

Checklist

The pull request title is a good summary of the changes - it will be used in the changelog
Unit tests for the changes exist
Tests pass on CI
Documentation reflects the changes where applicable
My PR is ready to review,please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer:@sydney-runkle

Add validation tests for_internal/_validators.py

ba8aa57

This PR introduces new unit tests to enhance the test coverage by covering the following scenarios:- Added a type check for decimal validators and the corresponding unit tests.- Added unit tests to validate numeric constraints (e.g., 'gt', 'lt', etc.).- Added a unit test to validate the `Fraction` type.

github-actionsbot added the relnotes-fixUsed for bugfixes. label

Nov 4, 2024

Copy link

codspeed-hqbot commentedNov 4, 2024•
edited
Loading

CodSpeed Performance Report

Merging#10763 willnot alter performance

_{Comparingtkasuz:increase_types_test_coverage (57783eb) withmain (bcfd413)}

Summary

✅ 44 untouched benchmarks

Copy link

Contributor

github-actionsbot commentedNov 4, 2024•
edited
Loading

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic/_internal
_validators.py
Project Total

_{This report was generated bypython-coverage-comment-action}

Copy link

ContributorAuthor

tkasuz commentedNov 4, 2024

please review

pydantic-hookybot added the ready for review label

Nov 4, 2024

pydantic-hookybot assignedsydney-runkle

Nov 4, 2024

sydney-runkle suggested changes

Nov 14, 2024

View reviewed changes

Copy link

Contributor

sydney-runkle left a comment

Choose a reason for hiding this comment

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

Thanks for helping us to bump up our test coverage!

I've left a few recommendations. Let me know if you have any follow up inquiries :)

tests/test_types.py OutdatedShow resolvedHide resolved

tests/test_validators.py OutdatedShow resolvedHide resolved

tests/test_types.py OutdatedShow resolvedHide resolved

pydantic/_internal/_validators.py Outdated

Comment on lines 391 to 392

		if not isinstance(x, Decimal):
		raise TypeError(f'Expected Decimal, received {type(x).__name__}')

Copy link

Contributor

sydney-runkleNov 14, 2024

Choose a reason for hiding this comment

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

Do we test this anywhere?

Copy link

ContributorAuthor

tkasuzNov 15, 2024•
edited
Loading

Choose a reason for hiding this comment

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

I ended up checking the Decimal type here instead

pydantic/pydantic/_internal/_validators.py

Line 374 inf6919b5

raiseTypeError(f'Unable to extract decimal digits info from supplied value{decimal}')

pydantic-hookybot added awaiting author revisionawaiting changes from the PR author and removed ready for review labels

Nov 14, 2024

pydantic-hookybot assignedtkasuz and unassignedsydney-runkle

Nov 14, 2024

consolidates some tests and uses try-except for _extract_decimal_digi…

f6919b5

…ts_infoSigned-off-by: tkasuz <63289889+tkasuz@users.noreply.github.com>

tkasuz force-pushed theincrease_types_test_coverage branch from0bbfcd5 tof6919b5Compare

November 15, 2024 04:31

tkasuz requested a review fromsydney-runkle

November 15, 2024 04:44

Fix 'type' object is not subscriptable error for Python3.8

57783eb

Signed-off-by: tkasuz <63289889+tkasuz@users.noreply.github.com>

sydney-runkle reviewed

Nov 15, 2024

View reviewed changes

Copy link

Contributor

sydney-runkle left a comment

Choose a reason for hiding this comment

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

Other than that one question, tests look great! Thanks for your contribution!

pydantic/_internal/_validators.py

Comment on lines -349 to -370

		decimal_tuple = decimal.as_tuple()
		if not isinstance(decimal_tuple.exponent, int):
		raise TypeError(f'Unable to extract decimal digits info from supplied value {decimal}')
		exponent = decimal_tuple.exponent
		num_digits = len(decimal_tuple.digits)

		if exponent >= 0:
		# A positive exponent adds that many trailing zeros
		# Ex: digit_tuple=(1, 2, 3), exponent=2 -> 12300 -> 0 decimal places, 5 digits
		num_digits += exponent
		decimal_places = 0
		else:
		# If the absolute value of the negative exponent is larger than the
		# number of digits, then it's the same as the number of digits,
		# because it'll consume all the digits in digit_tuple and then
		# add abs(exponent) - len(digit_tuple) leading zeros after the decimal point.
		# Ex: digit_tuple=(1, 2, 3), exponent=-2 -> 1.23 -> 2 decimal places, 3 digits
		# Ex: digit_tuple=(1, 2, 3), exponent=-4 -> 0.0123 -> 4 decimal places, 4 digits
		decimal_places = abs(exponent)
		num_digits = max(num_digits, decimal_places)
		try:
		decimal_tuple = decimal.as_tuple()

		return decimal_places, num_digits

Copy link

Contributor

sydney-runkleNov 15, 2024

Choose a reason for hiding this comment

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

Can we leave this as is? WhatAttributeError might occur here?

Copy link

ContributorAuthor

tkasuzNov 16, 2024

Choose a reason for hiding this comment

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

If thedecimal argument is not aDecimal type, which is accepted as Any in the root function, anAttributeError might occur because the as_tuple method may not exist

Copy link

ContributorAuthor

tkasuzNov 16, 2024

Choose a reason for hiding this comment

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

Would it be better to change the argument type toAny and add anotherassert to check if the type isDecimal? 🤔

Copy link

Contributor

sydney-runkleNov 16, 2024

Choose a reason for hiding this comment

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

Makes sense, thanks for the improvement!

sydney-runkle approved these changes

Nov 16, 2024

View reviewed changes

Copy link

Contributor

sydney-runkle left a comment

Choose a reason for hiding this comment

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

Thanks! Great work! 🚀

sydney-runkle merged commit0157e34 intopydantic:main

Nov 16, 2024

53 checks passed

Labels

awaiting author revision

awaiting changes from the PR author

relnotes-fix

Used for bugfixes.

2 participants

Movatterモバイル変換

Uh oh!

Add validation tests for_internal/_validators.py#10763

Add validation tests for_internal/_validators.py#10763

Uh oh!

Conversation

tkasuz commentedNov 4, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Change Summary

Related issue number

Checklist

Uh oh!

codspeed-hqbot commentedNov 4, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging#10763 willnot alter performance

Summary

Uh oh!

github-actionsbot commentedNov 4, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Coverage report

Uh oh!

tkasuz commentedNov 4, 2024

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sydney-runkleNov 14, 2024

Choose a reason for hiding this comment

Uh oh!

tkasuzNov 15, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

sydney-runkleNov 15, 2024

Choose a reason for hiding this comment

Uh oh!

tkasuzNov 16, 2024

Choose a reason for hiding this comment

Uh oh!

tkasuzNov 16, 2024

Choose a reason for hiding this comment

Uh oh!

sydney-runkleNov 16, 2024

Choose a reason for hiding this comment

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Add validation tests for`_internal/_validators.py`#10763

Add validation tests for`_internal/_validators.py`#10763

tkasuz commentedNov 4, 2024•
edited
Loading

codspeed-hqbot commentedNov 4, 2024•
edited
Loading

github-actionsbot commentedNov 4, 2024•
edited
Loading

tkasuzNov 15, 2024•
edited
Loading