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

run: don't change the value of i #727

Merged
merged 1 commit into from
May 25, 2023

Conversation

kolyshkin
Copy link
Contributor

I came across an interesting bug today. A very simple test code stopped working properly once I changed "run" followed by the $status check to "run !".

Fixes: #726 (see for a repro).

The fix is to always declare your variables.

@kolyshkin kolyshkin marked this pull request as ready for review May 25, 2023 03:23
@kolyshkin kolyshkin requested a review from a team as a code owner May 25, 2023 03:23
kolyshkin added a commit to kolyshkin/cri-o that referenced this pull request May 25, 2023
Instead of using

	run cmd
	[ "$status" -ne 0 ]

since bats 1.5.0 we can use a simple form:

	run ! cmd

Alas, bats has a bug which makes it change the value of "i" when run is
used with any options ([1], [2]). To workaround that, we should not use
"i" in any code which uses "run" with options. I know, this is crazy.

[1] bats-core/bats-core#726
[2] bats-core/bats-core#727

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/cri-o that referenced this pull request May 25, 2023
Instead of using

	run cmd
	[ "$status" -ne 0 ]

since bats 1.5.0 we can use a simple form:

	run ! cmd

Alas, bats has a bug which makes it change the value of "i" when run is
used with any options ([1], [2]). To workaround that, we should not use
"i" in any code which uses "run" with options. I know, this is crazy.

[1] bats-core/bats-core#726
[2] bats-core/bats-core#727

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

Adding a test case is useless, of course, since next time someone will change j instead of i. But I wrote some code to detect such things in general which might be useful; see the #726 (comment)

kolyshkin added a commit to kolyshkin/cri-o that referenced this pull request May 25, 2023
Instead of using

	run cmd
	[ "$status" -ne 0 ]

since bats 1.5.0 we can use a simple form:

	run ! cmd

Alas, bats has a bug which makes it change the value of "i" when run is
used with any options ([1], [2]). To workaround that, we should not use
"i" in any code which uses "run" with options. I know, this is crazy.

[1] bats-core/bats-core#726
[2] bats-core/bats-core#727

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@thaJeztah
Copy link

One failure in CI (no idea if related in any way);

not ok 324 temp_make() <var>: returns 1 and displays an error message if the directory can not be created
# (in test file bats-file/test/70-temp-10-temp_make.bats, line 32)
#   `[[ ${lines[1]} =~ $REGEX ]] || false' failed
# Last output:
#
# -- ERROR: temp_make --
# mktemp: No such file or directory
# --

@martin-schulze-vireso
Copy link
Member

martin-schulze-vireso commented May 25, 2023

I am not sure what to make of this. The issue seems to be persistent but I cannot reproduce it locally.

It seems the output from mktemp changed (again) in alpine. I fixed the test expecation in bats-core/bats-file@c0f822a

I came across an interesting bug today. A very simple test code stopped working
properly once I changed "run" followed by the $status check to "run !".

See bats-core#726 for a repro.

The fix is to always declare your variables.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@martin-schulze-vireso martin-schulze-vireso merged commit 3e30d37 into bats-core:master May 25, 2023
4 of 5 checks passed
@martin-schulze-vireso
Copy link
Member

Thanks for your contribution.

kolyshkin added a commit to kolyshkin/cri-o that referenced this pull request May 30, 2023
Instead of using

	run cmd
	[ "$status" -ne 0 ]

since bats 1.5.0 we can use a simple form:

	run ! cmd

Alas, bats has a bug which makes it change the value of "i" when run is
used with any options ([1], [2]). To workaround that, we should not use
"i" in any code which uses "run" with options. I know, this is crazy.

[1] bats-core/bats-core#726
[2] bats-core/bats-core#727

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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.

run with options overwrites the value of i
3 participants