-
Notifications
You must be signed in to change notification settings - Fork 3k
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
test: fix stats depth variable scope #4933
Conversation
The line is needed to be able to get different levels of stats when compiling tests (default is 2).
IMO it should continue to be passed. I'd suggest checking whether the variable exists. If doesn't, then assign a value (e.g. 2 is the default level of depth for memap stats). @theotherjimmy may have another suggestion. |
That is how is it with this patch, it is not passed, thus using the default value. I am not familiar with the changes for memmap, thus not certain how it can be passed to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this is the right way to go about this. Instead of allowing stats_depth
to take its default, we should find where it should have been passed in and pass it appropriately.
@theotherjimmy note to self: help with this. |
👍 You can push to my branch or just send a new PR. It would be nice to have this fixed. I'll talk to you today |
8f03d65
to
b99e6b5
Compare
Thanks @theotherjimmy |
I tested with singletest.py, and it seems to work. |
Tested locally, working for me as well |
👍 |
As this variable is not in scope, should not be referenced.
Fixes #4392No, you can't do "fixes PR#" only "fixes issue#" makes senseFixes #4930
I am not certain this is a proper fix. The variable is not in the scope in execute() . Should it be passed somehow there, or just ignored ? This fixes it as we are not using undefined variable. The question is how it should be passed to execute function or should not and this is correct?@theotherjimmy update: I passed stats depth correctly.
Tested with singletest.py. With this patch, it builds. Without, build does not even start, it crashes silently - FAIL build.
@MarceloSalazar @theotherjimmy You might know better, replace this patch or push to my branch.