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

Fix multiple invocations of the before function and before tracing #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix multiple invocations of the before function and before tracing #23

wants to merge 1 commit into from

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Jul 25, 2012

  1. The before function had been called twice, once with proper tracing and error
    output redirection and once directly. This is fixed now.

  2. The result output was moved out of the inner subshell. This is necessary
    because: in case the before function has an error, roundup's error trap is
    triggered and the inner subshell is left. Still, we need a proper result ouput.

  3. A test case with a broken before function was added which gives
    the following output:

    roundup before trace
    it_works: [FAIL]
    + false
    exit code 1

for the following before function:

before () {
false
}

1. The `before` function had been called twice, once with proper tracing and error
output redirection and once directly. This is fixed now.

2. The result output was moved out of the inner subshell. This is necessary
because: in case the `before` function has an error, roundup's error trap is
triggered and the inner subshell is left. Still, we need a proper result ouput.

3. A test case with a broken `before` function was added which gives
the following output:

  roundup before trace
    it_works:                                        [FAIL]
      + false
      exit code 1

for the following before function:

  before () {
    false
  }
@bmizerany
Copy link
Owner

There are many things I now think are not as neccessary to roundup as I once thought. before is one of them. It adds a lot of complexity that can be avoided by simply having the user create their own before and call it at the start of whichever tests they need it. This goes for after as well. I'd love to hear your thoughts on this.

@sttts
Copy link
Contributor Author

sttts commented Dec 7, 2012

We use before in all of our tests to set a certain environments. The added value is limited of course, compared to the number of lines the before support needs. But this is mainly due to duplicated code for tracing of before and the testcase itself. I am sure this can be implemented much simpler by factoring out the tracing code. I agree that it does not look nice right now and needs a refactoring to get back the former elegance and simplicity.

If you look at my branch now, you will even find an init/cleanup mechanism which acts like global before/after for the whole file. I don't like very much how it is implemented right now in that branch. But init/cleanup really brings additional value because you cannot easily simulate this without support in roundup itself.

However, coming back to this commit: this patch fixes the before mechanism as it is in your master right now. The only complexity it adds is the exit $roundup_result line and the move of the result output code out of the bracket. Otherwise, the added code lines are in the test cases.

@edysli
Copy link

edysli commented Jan 7, 2013

This is an embarrassing bug. :( I keep using commit a8a175a before this function was broken.

@larsks
Copy link

larsks commented Feb 7, 2013

So...I just found roundup, and it seemed awesome, but then my test setup/teardown didn't seem to be working correctly. I think it's this same issue; if I log execution of my before/after functions, I see this for a testplan with two tests:

run before
run before
run after
run before
run before
run after

...so yeah, it's running the before function twice. Having a fix for this would be great. I wouldn't want to see it go away...having cleanup/teardown functions is really quite nice.

LukeShu added a commit to LukeShu/roundup that referenced this pull request Apr 11, 2017
See the comments for a fuller explanation of the changes.

This should obsolete bmizerany#23 (though
the test cases from it should still be merged).

This should fix bmizerany#29 .

This should fix bmizerany#44 .
LukeShu pushed a commit to LukeShu/roundup that referenced this pull request Apr 11, 2017
Fix multiple invocation of the `before` function and `before` tracing

1. The `before` function had been called twice, once with proper tracing and error
output redirection and once directly. This is fixed now.

2. The result output was moved out of the inner subshell. This is necessary
because: in case the `before` function has an error, roundup's error trap is
triggered and the inner subshell is left. Still, we need a proper result ouput.

3. A test case with a broken `before` function was added which gives
the following output:

  roundup before trace
    it_works:                                        [FAIL]
      + false
      exit code 1

for the following before function:

  before () {
    false
  }
LukeShu added a commit to LukeShu/roundup that referenced this pull request Apr 11, 2017
See the comments for a fuller explanation of the changes.

This should obsolete bmizerany#23 (though
the test cases from it should still be merged).

This should fix bmizerany#29 .

This should fix bmizerany#44 .
LukeShu pushed a commit to LukeShu/roundup that referenced this pull request Apr 11, 2017
Fix multiple invocation of the `before` function and `before` tracing

1. The `before` function had been called twice, once with proper tracing and error
output redirection and once directly. This is fixed now.

2. The result output was moved out of the inner subshell. This is necessary
because: in case the `before` function has an error, roundup's error trap is
triggered and the inner subshell is left. Still, we need a proper result ouput.

3. A test case with a broken `before` function was added which gives
the following output:

  roundup before trace
    it_works:                                        [FAIL]
      + false
      exit code 1

for the following before function:

  before () {
    false
  }
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.

4 participants