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

Fix TypeError on YAML test execution with trace option #1299

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

sandcha
Copy link
Collaborator

@sandcha sandcha commented Nov 19, 2024

Connected to #1293 issue
Connected to openfisca/openfisca-france#2387 issue

And contains same fix as in #1304 issue (adds tests)

We can merge #1304 before looking at this PR for the reactivated and added minimal test(s)

Bug fix

  • Fix print_computation_log() for YAML tests with verbose option
    • Sets a default value for max_depth and fixes TypeError on openfisca test -v file.yaml command introduced in openfisca-core 41.5.6

@sandcha
Copy link
Collaborator Author

sandcha commented Nov 25, 2024

Rebasing to get #1297. This should fix current TypeError: '>' not supported between instances of 'int' and 'NoneType' (detected for example by these GitHub actions checks).

@sandcha sandcha force-pushed the fix-typeerror-tracer branch from 7076ff3 to 08c878a Compare November 25, 2024 18:31
@sandcha
Copy link
Collaborator Author

sandcha commented Nov 26, 2024

Rebasing on top of #1297 didn't fix the issue: max_depth was set to a high default value but it was forgotten in YamlItem.runtest(...) where the max_depth command line option was used. So, a command line that didn't set the max_depth option ended up with max_depth set to None and the TypeError described above.

Fixed in last commits and the same fix also comes with open #1304.

🐛 The issue seems to have started with #1185:
see line 64 in openfisca_core/tracers/computation_log.py file from diff.


def _get_node_log(
self,
node: t.TraceNode,
depth: int,
aggregate: bool,
max_depth: int = sys.maxsize,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Default value removed as _get_node_log shouldn't be called outside openfisca_core (as its name starting with _ suggests). So wouldn't a change of the max_depth option value here produce unexpected behaviors for code calling the public openfisca_core?

super().__init__("", TestFile(), TaxBenefitSystem(), test, {})

self.tax_benefit_system = self.baseline_tax_benefit_system
self.simulation = Simulation()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed to call the default constructor. Simulation() used once only in test_variable_not_found.

@@ -191,6 +230,33 @@ def test_performance_tables_option_output() -> None:
clean_performance_files(paths)


def test_verbose_option_output(capsys) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The test used to debug the TypeError fixed by this PR.

@sandcha sandcha marked this pull request as ready for review November 26, 2024 19:25
Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

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

Looks good but since there are many things I do not understand, I will let the last word to pros like @bonjourmauko or @guillett

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.

2 participants