-
Notifications
You must be signed in to change notification settings - Fork 587
Refactor reproduce #4988
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
base: master
Are you sure you want to change the base?
Refactor reproduce #4988
Conversation
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.
lgtm
|
||
# Make fuzzer executable. | ||
os.chmod(fuzzer_path, 0o750) | ||
os.chmod(fuzzer_path, _EXECUTABLE_PERMISSIONS) |
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.
nice
upload_url: str): | ||
def archive_testcase_and_dependencies_in_gcs( | ||
resource_list, | ||
testcase_path: str, # pylint: disable=line-too-long |
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 think this line-too-long can be removed. ?
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.
You're right. I've removed and runned lint again. Thanks!
Could you provide a quick evidence of these changes working? |
|
||
try: | ||
_, testcase_file_path = _get_testcase_file_and_path(testcase) | ||
if testcase.minimized_keys and testcase.minimized_keys != 'NA': |
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.
Why not use the _is_testcase_minimized
method here?
|
||
def set_local_log_only(): | ||
"""Force logs to be local-only.""" | ||
set_value('LOG_TO_GCP', '') |
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.
Please, add a short comment explaining that we set this to an empty string because currently the logs module does not correctly evaluate env vars (i.e., 'false' is evaluated to true).
return archived, absolute_filename, zip_filename | ||
|
||
|
||
def setup_local_testcase(testcase: data_types.Testcase) -> str | None: |
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.
There is a preprocess_setup_testcase
method which you don't define a local version. Is there a reason?
Also, I noticed that in this function, it sets the APP_ARGS
based on the return of _get_application_arguments
, which you do only in the reproduce module. Maybe you could try to do it here instead and using this helper method?
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
"""Tests for the reproduce butler.""" |
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.
go/pystyle#test-docs
This PR is a follow-up to #4976, addressing the feedback and discussions from the original review. The primary focus is on refactoring and code quality improvements.
(Note: This PR involves a significant amount of code being moved to more appropriate modules.)
Quick summary:
setup.py
module--local-logging
flag tobutler
to enable convenient local log output during development.minimized_keys
during setup.