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

Fds 2333 validate attribute unit tests #1517

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

Conversation

andrewelamb
Copy link
Contributor

@andrewelamb andrewelamb commented Oct 4, 2024

  • Fixes FDS-2333
  • Adds unit tests for Methods of the ValidateAttribute class
  • Cleans up documentation for ValidateAttribute class

@@ -1147,6 +1152,7 @@ def type_validation(
)
if vr_errors:
errors.append(vr_errors)
# It seems impossible to get warnings with type rules
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have any follow-up tech debt ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think so. I wanted to mention it here to see if I misunderstood something first.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewelamb I'm not sure where I'm seeing where it's impossible, could you explain in case I missed something? The default message for all the type rules is error but for every validation rule a user could specify in their data model to specifically raise warnings or error regardless of the default level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GiaJordan that's doesn't appear to be true. See here.

"Use the type checker by setting the ‘Validation Rules’ to one of the types listed below. No additional specifiers are allowed. "

If you look at the code in the validator above that is confirmed. Unless the val_rule is one of ["num", "str", "int", "float"] you'll just get empty errors and warnigns back.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewelamb that does look like the current behavior, I don't think that's intended though, but that would probably be another discussion. Message level specifications are allowed in the rule definitions in validation_rule_info()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll start a jira issue then.

Comment on lines +1561 to +1564

##############
# get_no_entry
##############
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of grouping by comment like this should this be a new class?

class TestGetNoEntry:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm not sure. Do other egineers have preferences for this?

Copy link
Contributor

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

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

Great additions to the tests! A few comments, but they're all trivial concerns.

Copy link

sonarcloud bot commented Oct 12, 2024

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

Successfully merging this pull request may close these issues.

3 participants