Skip to content

Conversation

mbland
Copy link
Contributor

@mbland mbland commented May 30, 2018

Closes #34. Incorporates the test code from that PR.

This fixes a problem whereby invoking an exported function within a test script would result in an incorrect stack trace. It does this by using BATS_TEST_SOURCE to capture a function's frame when its corresponding BASH_SOURCE entry is empty.

Note: I believe the earlier problem described in sstephenson/bats#225 whereby no output was generated at all was fixed by commit 0f6dde5.

My investigation revealed that in Bash versions all the way up through 4.3.48, before this code change, the test failed as @pawamoy reported, with the underlying test fixture producing the following output:

   not ok 1 failing test
     (in file , line 7,
      from function `bats_perform_test' in file libexec/bats-exec-test, line 339,
      from function `main' in test file , line 391)

Starting with Bash 4.4, the test passed.

The apparent reason for the failure is that the BASH_SOURCE entry corresponding to the test file was getting wiped out when the exported function was called. (Once I converged on this as a potential culprit, I verified it by printing the contents of BASH_SOURCE before and after the exported function call.) Since bats_capture_stack_trace depends on matching a BASH_SOURCE value against BATS_TEST_SOURCE, when the BASH_SOURCE corresponding to the test function was empty, it would keep collecting stack frames, resulting in the output above.

As best I can currently tell, the change accounting for the fix in Bash 4.4 is (from the Bash 4.4 ChangeLog):

  1/2/2014
  --------
  ...snip...

  2/26
  ----
  [bash-4.3 released]
  ...snip...

  11/28
  -----
  execute_cmd.c
    - struct func_array_state: new state to save state of BASH_LINENO,
      BASH_SOURCE, and FUNCNAME during function execution so it can be
      restored on a jump to top level
    - restore_funcarray_state: new function to restore func_array_state
    - execute_function: fill in func_array_state variable, add unwind-
      protect to restore it on jump to top level, call explicitly at
      end of function if subshell != 0 (may not be necessary, but safe
      for now).  Fixes bug with local assignments to FUNCNAME reported
      by Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>

  1/1/2015
  --------
  ...snip...

  7/7
  ---
  ...snip...
  [bash-4.4-alpha frozen]

Closes #34. Incorporates the test code from that PR.

This fixes a problem whereby invoking an exported function within a test
script would result in an incorrect stack trace. It does this by using
`BATS_TEST_SOURCE` to capture a function's frame when its corresponding
`BASH_SOURCE` entry is empty.

Note: I believe the earlier problem described in sstephenson/bats#225
whereby no output was generated at all was fixed by commit
0f6dde5.

My investigation revealed that in Bash versions all the way up through
4.3.48, before this code change, the test failed as @pawamoy reported,
with the underlying test fixture producing the following output:

   not ok 1 failing test
     (in file , line 7,
      from function `bats_perform_test' in file libexec/bats-exec-test, line 339,
      from function `main' in test file , line 391)

Starting with Bash 4.4, the test passed.

The apparent reason for the failure is that the `BASH_SOURCE` entry
corresponding to the test file was getting wiped out when the exported
function was called. (Once I converged on this as a potential culprit, I
verified it by printing the contents of `BASH_SOURCE` before and after
the exported function call.) Since `bats_capture_stack_trace` depends on
matching a `BASH_SOURCE` value against `BATS_TEST_SOURCE`, when the
`BASH_SOURCE` corresponding to the test function was empty, it would
keep collecting stack frames, resulting in the output above.

As best I can currently tell,  the change accounting for the fix in Bash
4.4 is (from the Bash 4.4 ChangeLog):

  1/2/2014
  --------
  ...snip...

  2/26
  ----
  [bash-4.3 released]
  ...snip...

  11/28
  -----
  execute_cmd.c
    - struct func_array_state: new state to save state of BASH_LINENO,
      BASH_SOURCE, and FUNCNAME during function execution so it can be
      restored on a jump to top level
    - restore_funcarray_state: new function to restore func_array_state
    - execute_function: fill in func_array_state variable, add unwind-
      protect to restore it on jump to top level, call explicitly at
      end of function if subshell != 0 (may not be necessary, but safe
      for now).  Fixes bug with local assignments to FUNCNAME reported
      by Arfrever Frehtes Taifersar Arahesis <arfrever.fta@gmail.com>

  1/1/2015
  --------
  ...snip...

  7/7
  ---
  ...snip...
  [bash-4.4-alpha frozen]
@mbland mbland requested review from jasonkarns, sublimino, btamayo and a team May 30, 2018 22:20
@ghost ghost assigned mbland May 30, 2018
@ghost ghost added the review label May 30, 2018
@mbland
Copy link
Contributor Author

mbland commented May 30, 2018

Hmm, failing on the Alpine Docker image. Will take a look later.

mbland added 3 commits May 30, 2018 22:32
This makes it faster to iterate on running changes to Bats inside the
container, since the apk packages are far less likely to change.
This allows the Docker tests to run even if the native tests fail,
providing more information.
This fixes the build failure seen during #87 from the "execute exported
function without breaking failing test output" test case.

The failure was due to the Dockerfile copying the Bats files to
/opt/bats, rendering the equality expression too rigid. The expression
now uses a regex comparison, allowing the test to pass both natively and
when running in the container.
@mbland
Copy link
Contributor Author

mbland commented May 31, 2018

OK, pushed a commit to fix the Alpine Docker run: The equality expression checking the expected stack trace was too rigid; changed it to a regex.

Also pushed a commit to add Bash to the Alpine container before copying the Bats files (makes for faster local development) and a commit to run the native and Docker tests as two separate script: steps (to get more information from the Docker run even if the native run fails).

Should've done this in the previous commit rather than using a regex,
but I'd forgotten until I looked over other test cases after-the-fact.

This allows the "execute exported function without breaking failing test
output" to pass both locally/natively and in the Alpine Docker
container.
@mbland
Copy link
Contributor Author

mbland commented May 31, 2018

D'oh! Forgot about RELATIVE_FIXTURE_ROOT until just now. Pushed another commit to use that variable instead of the regex to maintain compatibility between local/native and Alpine Docker runs.

@mbland
Copy link
Contributor Author

mbland commented May 31, 2018

And today I just realized that the right-hand side of = and == are treated as glob patterns, meaning that I could've used a * in the test assertion instead of RELATIVE_FIXTURE_ROOT. Learn something new everyday! Still, RELATIVE_FIXTURE_ROOT provides a stricter assertion while allowing for portability, so it's preferable.

@mbland
Copy link
Contributor Author

mbland commented Jun 3, 2018

Going to merge this now; if there're any comments or concerns after-the-fact, I'm happy to address them.

@mbland mbland merged commit f837f3d into master Jun 3, 2018
@ghost ghost removed the review label Jun 3, 2018
@mbland mbland deleted the exported-function-fix-#34 branch June 3, 2018 19:12
@mbland mbland mentioned this pull request Jun 3, 2018
2 tasks
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.

1 participant