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

[feat] Enable default value type checking and implicit conversions at the variable declaration level #3115

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

vkarak
Copy link
Contributor

@vkarak vkarak commented Feb 12, 2024

Currently, the default values of variables or the values assigned to them in the test class body are not type checked and no implicit conversions are applied to the value being assigned. This is deferred at the class instantiation time where the default values of variables are injected to the newly created test object and type checked through the associated descriptor.

This PR moves the type checking and implicit conversions, i.e., the functionality of the descriptors at the class level. We do so, with two tricks:

  1. We extend the Python descriptor model by having __set__() return the value that the descriptor would write into the object if the object passed to __set__() is None.
  2. We anticipate the call to the descriptor's __set_name__() at the same time that this function is called for the variable. And it is at this point that we also call the descriptor's __set__() method to force the type checking and any type conversion. For type conversions we do respect the allow_implicit argument of variables.

The advantages of this method are

  • Variables before and after the test's instantiation behave the same way.
  • Type errors during variable assignments are now more precise as the source file information is included in the error message.

Todos

  • Remove unnecessary conversions and type checking (type checking in inject() should not be needed)
  • Double-check the setvar() impl. for redundant operations.

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e7e93f0) 86.67% compared to head (317fef3) 86.66%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3115      +/-   ##
===========================================
- Coverage    86.67%   86.66%   -0.02%     
===========================================
  Files           61       61              
  Lines        12082    12092      +10     
===========================================
+ Hits         10472    10479       +7     
- Misses        1610     1613       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vkarak vkarak marked this pull request as ready for review February 12, 2024 17:02
@vkarak
Copy link
Contributor Author

vkarak commented Feb 12, 2024

The test instantiation time increases by about 5% and the overall time of the following benchmark increases by 3.5%:

reframe -c unittests/resources/checks/hellocheck.py -n ^HelloTest --repeat=1000 -l

I believe the difference comes from the fact that we now validate every assignment of a variable, whereas before we were validating once during test instantiation.

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

lgtm

@vkarak vkarak merged commit 8395b49 into reframe-hpc:develop Feb 20, 2024
24 of 25 checks passed
@vkarak vkarak deleted the feat/vars-implicit-conversions branch February 20, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants