-
Notifications
You must be signed in to change notification settings - Fork 790
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
Upgrade typeguard, and add test that would have caught typeguard-related regression #2241
base: master
Are you sure you want to change the base?
Conversation
97c5c14
to
ed0efe2
Compare
8284d20
to
863e9b1
Compare
Refactored this, adding a One weakness here is that these tests would all pass if the runtime parameters weren't applied. I thought a little about how to prove that they are being applied. For example, if you could detect from the test context whether we were running via the cli executor vs the api executor, then you could set a parameter differently in PARAMETERS vs RUNTIME_PARAMETERS and test that you've gotten the expected one. But I didn't find a good solution. |
Thanks for the contribution! I haven't looked in detail yet but a few high level comments:
|
3f82469
to
57bf1fd
Compare
Thanks kindly @romain-intel. Pushed a couple commits:
|
This refers to issue #2235.
The proximal issue is that you can't use a
JSONType
parameter via the Runner API with recent python versions.That raises an error from the vendored-in
typeguard
library, which performs runtime type checks. That error arises in turn from updates to cpython, that made a previously-positional argument required to be specified as a keyword. Typeguard has since been patched to address this issue.This PR aims to update the typeguard dependency, and to add a test that would have caught the issue when it first arose.
Adding the test is the more challenging problem. The current test runner invokes the API tests like so:
To test an additional parameter via the path that has failed here, one would need somehow to get it into the
run_level_dict
.I'm proposing here to augment the
run_level_dict
parameters via aRUNTIME_PARAMETERS
environment variable that may be set in a test.Running via:
After setting
RUNTIME_PARAMETERS
now yields a test failure; the same "recursive_guard" issue under discussion.With that in place, I patched typeguard, and that resolves the test failure.
I also ran
PYTHONPATH=
pwd/../../ python run_tests.py --debug --contexts dev-local --tests BasicParameterTest
, and those passed as well. I might be wrong, but I'm expecting that the GH Actions would be relied upon to test other configurations.They may well be a cleaner way to achieve test coverage of this code path, and I'm happy to modify/iterate if so. Hopefully this at least helps identify the key issues.