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

Always use bash arrays for changed files and directories #4703

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

pavoljuhas
Copy link
Collaborator

@pavoljuhas pavoljuhas commented Nov 23, 2021

Store changed paths in bash arrays by splitting diff output at newlines.
Avoid scalars that need to be split and may perform wildcard expansion.

Ref: https://www.baeldung.com/linux/reading-output-into-array#using-theread-command

Store changed paths in bash arrays by splitting diff output at newlines.
Avoid scalars that need to be split and that may do wildcard expansion.
@pavoljuhas pavoljuhas requested review from cduck, vtomole and a team as code owners November 23, 2021 20:13
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Nov 23, 2021
@pavoljuhas pavoljuhas requested a review from dabacon November 23, 2021 20:13
@CirqBot CirqBot added the size: S 10< lines changed <50 label Nov 23, 2021
@pavoljuhas
Copy link
Collaborator Author

Here is a patch that I used to make dummy source code changes and test the modified check/*-changed-files* scripts - check-bash-array-values.patch.txt
Below I am including script outputs before and after this PR. Note that declare sections show multi-line scalars in the initial version and a correctly set bash arrays after the update. This also fixes the ERROR: file or directory not found that happens for the initial pytest-changed-files-and-incremental-coverage script.

initial scripts from master at 42ac9da

$ git checkout 42ac9daf 
$ git apply check-bash-array-values.patch.txt

$ check/pytest-changed-files
Comparing against revision 'origin/master'.
# ----------------------------------------------------------------------
declare -- changed="cirq-core/cirq/ops/tags_test.py
cirq-rigetti/cirq_rigetti/sampler_test.py"
# ----------------------------------------------------------------------
Found 2 test files associated with changes.
...


$ check/pylint-changed-files
Comparing against revision 'origin/master'.
# ----------------------------------------------------------------------
declare -- changed="cirq-core/cirq/ops/tags_test.py
cirq-rigetti/cirq_rigetti/sampler_test.py"
# ----------------------------------------------------------------------
Found 2 lintable files associated with changes.


$ check/pytest-changed-files-and-incremental-coverage
Comparing against revision 'origin/master'.
# ----------------------------------------------------------------------
declare -- cov_changed_python_file_dirs="--cov=cirq-core/cirq/ops
--cov=cirq-rigetti/cirq_rigetti"
# ----------------------------------------------------------------------
declare -- changed_python_tests="cirq-core/cirq/ops/tags_test.py
cirq-rigetti/cirq_rigetti/sampler_test.py"
# ----------------------------------------------------------------------
...
ERROR: file or directory not found: cirq-core/cirq/ops/tags_test.py
cirq-rigetti/cirq_rigetti/sampler_test.py

same tests with updated scripts from this PR

$ git checkout fix-bash-array-assignment
$ git apply -3 check-bash-array-values.patch.txt; git reset

$ check/pytest-changed-files
Comparing against revision 'origin/master'.
# ----------------------------------------------------------------------
declare -a changed=([0]="cirq-core/cirq/ops/tags_test.py" [1]="cirq-rigetti/cirq_rigetti/sampler_test.py")
# ----------------------------------------------------------------------
Found 2 test files associated with changes.
...


$ check/pylint-changed-files
Comparing against revision 'origin/master'.
# ----------------------------------------------------------------------
declare -a changed=([0]="cirq-core/cirq/ops/tags_test.py" [1]="cirq-rigetti/cirq_rigetti/sampler_test.py")
# ----------------------------------------------------------------------
Found 2 lintable files associated with changes.


$ check/pytest-changed-files-and-incremental-coverage
Comparing against revision 'origin/master'.
# ----------------------------------------------------------------------
declare -a cov_changed_python_file_dirs=([0]="--cov=cirq-core/cirq/ops" [1]="--cov=cirq-rigetti/cirq_rigetti")
# ----------------------------------------------------------------------
declare -a changed_python_tests=([0]="cirq-core/cirq/ops/tags_test.py" [1]="cirq-rigetti/cirq_rigetti/sampler_test.py")
# ----------------------------------------------------------------------
============================================ test session starts =============================================
platform linux -- Python 3.9.7, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /usr/local/google/home/juhas/Code/gh/quantumlib/Cirq
plugins: xdist-2.2.1, forked-1.3.0, asyncio-0.16.0, cov-3.0.0
collected 2 items

cirq-core/cirq/ops/tags_test.py .                                                                      [ 50%]
cirq-rigetti/cirq_rigetti/sampler_test.py s                                                            [100%]The annotate command will be removed in a future version.
Get in touch if you still use it: [email protected]


============================================== warnings summary ==============================================
../../../../arch/pycq/lib/python3.9/site-packages/coverage/inorout.py:469
  /usr/local/google/home/juhas/arch/pycq/lib/python3.9/site-packages/coverage/inorout.py:469: CoverageWarning: --include is ignored because --source is set (include-ignored)
    self.warn("--include is ignored because --source is set", slug="include-ignored")

-- Docs: https://docs.pytest.org/en/stable/warnings.html

----------- coverage: platform linux, python 3.9.7-final-0 -----------
Coverage annotated source written next to source

================================== 1 passed, 1 skipped, 1 warning in 2.58s ===================================

All touched lines covered

Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM, it looks like similar patterns are being used in format-incremental, although it doesn't seem to have this same breakage (it will still find format bugs in both scenarios given from your comment). Might be worth updating for consistency sake.

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 29, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 29, 2021
@CirqBot CirqBot merged commit 5a30119 into quantumlib:master Nov 29, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Nov 29, 2021
@pavoljuhas pavoljuhas deleted the fix-bash-array-assignment branch November 30, 2021 19:56
@pavoljuhas
Copy link
Collaborator Author

LGTM, it looks like similar patterns are being used in format-incremental, although it doesn't seem to have this same breakage (it will still find format bugs in both scenarios given from your comment). Might be worth updating for consistency sake.

Sure, please see #4717

rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…4703)

Store changed paths in bash arrays by splitting diff output at newlines.
Avoid scalars that need to be split and may perform wildcard expansion.

Ref: https://www.baeldung.com/linux/reading-output-into-array#using-theread-command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants