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 with options overwrites the value of i #726

Closed
kolyshkin opened this issue May 25, 2023 · 4 comments · Fixed by #727
Closed

run with options overwrites the value of i #726

kolyshkin opened this issue May 25, 2023 · 4 comments · Fixed by #727
Labels
Priority: NeedsTriage Issue has not been vetted yet Type: Bug

Comments

@kolyshkin
Copy link
Contributor

Describe the bug

The value of a variable i gets overwritten if run uses any flags.

To Reproduce
Steps to reproduce the behavior:

  1. Create the file a.bats:
bats_require_minimum_version 1.5.0

@test i {
  for ((i=0; i<3; i++)) {
    echo before $i
    run ! false
    echo after $i
  }
}
  1. Run it:
bats --show-output-of-passing-tests a.bats
  1. See what happens:
   before 0
   after 2

Expected behavior

I have expected the following output:

   before 0
   after 0
   before 1
   after 1
   before 2
   after 2

Environment (please complete the following information):

  • Bats Version: 1.9.0
  • OS: Linux
  • Bash version: 5.2.15

Additional context
A variable i is not declared as local in a few functions in common.bash.

@kolyshkin kolyshkin added Priority: NeedsTriage Issue has not been vetted yet Type: Bug labels May 25, 2023
kolyshkin added a commit to kolyshkin/bats-core that referenced this issue May 25, 2023
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>
kolyshkin added a commit to kolyshkin/bats-core that referenced this issue May 25, 2023
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>
@kolyshkin
Copy link
Contributor Author

Should be fixed by #727 (tested locally).

kolyshkin added a commit to kolyshkin/cri-o that referenced this issue 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 issue 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

I was thinking about how to detect such thing (adding/replacing variables) in general, and wrote this:

@test varcheck {
	# collect variables
	before="$(declare -p)"
	
	# run some commands
	run true
	run ! false
	run -0 echo foo

	# check
	diff -u <(echo "$before" | grep -vE ' BATS_') <(declare -p | grep -vE ' before=|lines=|output=|status=| BATS_')
}

In my setup, it changes the value of SHELLOPTS and BASH_REMATCH, which is probably ok.

kolyshkin added a commit to kolyshkin/cri-o that referenced this issue 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>
martin-schulze-vireso pushed a commit to kolyshkin/bats-core that referenced this issue May 25, 2023
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
Copy link
Member

martin-schulze-vireso commented May 25, 2023

There is something similar in https://github.com/bats-core/bats-core/blob/3e30d37383db7e527f57d02101d84a4ffa9c30a2/test/bats.bats#LL1216 for the debug trap. With run and other bats internals like run it becomes harder to capture all options.

I'd hoped for shellcheck to catch this sort of problem, but there is koalaman/shellcheck#468

kolyshkin added a commit to kolyshkin/cri-o that referenced this issue 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>
@debarshiray
Copy link
Contributor

I spent the better part of a few days banging on the wall over this. In my case run --separate-stderr was always setting i to 1. That made my for loop run, which had a limit of 5 infinitely, and gave the impression that the test case was stuck.

This happened while I was cleaning up various rough edges in my test suite, and I kept thinking that I made a mistake somewhere until I took a close look at the value of the loop counter (ie., i). A bit of head scratching and some Git forensics brought me here. :)

Thanks!

debarshiray added a commit to debarshiray/toolbox that referenced this issue Sep 21, 2023
Until Bats 1.10.0, 'run' with options had a bug where it would overwrite
the value of the 'i' variable even outside 'run' [1].

In these particular instances, no options are being passed to 'run',
and, hence, currently there's no problem.  However, in case a future
commit adds an option, then it could lead to hard-to-debug problems.
eg., --separate-stderr sets 'i' to 1, --show-output-of-passing-tests
sets it to 2, etc..  Therefore, depending on the flag and the loop, the
loop might get terminated prematurely or run infinitely or something
else.

Moreover, Bats 1.10.0 is only available in Fedora >= 39 and is absent
from Fedoras 37 and 38.  Therefore, it's not possible to consider this
bug fixed.

Hence, it's better to preemptively work around it to avoid any future
issues.

[1] Bats commit 502dc47dd063c187
    bats-core/bats-core@502dc47dd063c187
    bats-core/bats-core#726
debarshiray added a commit to debarshiray/toolbox that referenced this issue Sep 21, 2023
Until Bats 1.10.0, 'run' with options had a bug where it would overwrite
the value of the 'i' variable even outside 'run' [1].

In these particular instances, no options are being passed to 'run',
and, hence, currently there's no problem.  However, in case a future
commit adds an option, then it could lead to hard-to-debug problems.
eg., --separate-stderr sets 'i' to 1, --show-output-of-passing-tests
sets it to 2, etc..  Therefore, depending on the flag and the loop, the
loop might get terminated prematurely or run infinitely or something
else.

Moreover, Bats 1.10.0 is only available in Fedora >= 39 and is absent
from Fedoras 37 and 38.  Therefore, it's not possible to consider this
bug fixed.

Hence, it's better to preemptively work around it to avoid any future
issues.

[1] Bats commit 502dc47dd063c187
    bats-core/bats-core@502dc47dd063c187
    bats-core/bats-core#726

containers#1373
debarshiray added a commit to debarshiray/toolbox that referenced this issue Sep 21, 2023
This should prevent these functions from overwriting variables of the
same name beyond the function and causing hard-to-debug problems [1].

[1] Bats commit 502dc47dd063c187
    bats-core/bats-core@502dc47dd063c187
    bats-core/bats-core#726

containers#1373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: NeedsTriage Issue has not been vetted yet Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants