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

Reverse suite afterEach behavior to match semantics? #908

Merged
merged 1 commit into from
Sep 2, 2015

Conversation

mcamac
Copy link
Contributor

@mcamac mcamac commented Aug 18, 2015

Hi, thanks for the great library! I've noticed something which I have a question about.

If there are multiple afterEaches in a suite, they get run in reverse order of declaration. The current test spec/core/integration/SpecRunningSpec.js appends afterEach2 before afterEach1, which is counter-intuitive to me since the test is named "should run multiple befores and afters in the order they are declared".

I've made changes to the Env src file and the corresponding spec - what do you think about this behavior change?

@bahmutov
Copy link

+1 - this makes the runtime order a lot more intuitive and avoids surprises. Also matches the execution in Mocha

@m1gu3l
Copy link

m1gu3l commented Aug 19, 2015

+1

slackersoft pushed a commit that referenced this pull request Sep 2, 2015
Reverse suite afterEach behavior to match semantics?
@slackersoft slackersoft merged commit 825dab3 into jasmine:master Sep 2, 2015
@m1gu3l
Copy link

m1gu3l commented Nov 17, 2015

When can next release be expected?

@slackersoft
Copy link
Member

We're working on it

@just-boris
Copy link

My tests just have been blown because of this change.

Is there any release note, where I could know it before it happened?

@just-boris
Copy link

My example is:

  1. I am using angular-mocks to test my application. It has a beforeEach and afterEach hooks with angular environment setup.
  2. Then have some my app-specific hooks, relied on angular-mocks stuff.

Everything worked fine, but after this change angular's afterEach is fired first. Then my hook is fired and it can't finish, because angular environment already has been disposed.

To work around it, now I have to split my helper file to two and place may afterEach first, and beforeEach as last.

'helpers-finish.js',
'angular-mocks.js',
'helpers-init.js'

This is an insane

@rymai
Copy link

rymai commented Dec 3, 2015

We just experienced the same issue. In our case, the afterEach of jasmine-flight was trigerred before our own afterEach in which we were accessing this.component which would have been already teardowned by jasmine-flight.

I agree that the reason for this change is good but in reality, this will probably break many test suites.

@just-boris
Copy link

Also found that mocha calls afterEach as you did it before. Now behaviour of these two tools are different. I think it is not good.

@slackersoft
Copy link
Member

@just-boris @rymai sorry for the breaking change, it wasn't intended. The primary reason we even considered this PR was the note from @bahmutov that mocha ran afterEach blocks in the new order.

If we are going to make this change (which your use cases seem to suggest it would be a bad idea) we should have done it as part of a major version bump.

I'm going to revert and drop a new release and we can have a bigger discussion about which ordering makes more sense.

slackersoft pushed a commit that referenced this pull request Dec 4, 2015
- Run `afterEach` in reverse order declared as before
@just-boris
Copy link

@slackersoft Thank you!
I am looking forward for future discussion

@rymai
Copy link

rymai commented Dec 4, 2015

Thanks @slackersoft! /cc @dharFr

@m1gu3l
Copy link

m1gu3l commented Dec 4, 2015

@just-boris

Also found that mocha calls afterEach as you did it before. Now behaviour of these two tools are different. I think it is not good.

mocha runs afterEach in order of declaration: http://codepen.io/anon/pen/PZoGvW
jasmine (every version except 2.4.0) runs afterEach in reversed order of declaration http://codepen.io/anon/pen/xZxRxv

I believe this should be unified, fix reintroduced and released as 3.0.0.

@just-boris
Copy link

@m1gu3l yeah, you are right.
I messed with it yesterday, sorry.

@amilligan
Copy link
Contributor

In this case the (original) Jasmine behavior is correct. As several people have discovered, setting up and tearing down the state of tests behaves somewhat like a stack: the last thing you set up should be the first thing you tear down. Ordering issues become very difficult to track otherwise, as you deal with various libraries, stubs, the clock, etc.

@slackersoft
Copy link
Member

I'm inclined to agree with @amilligan on ordering of afterEach invocations. Running them in the reverse declaration order allows libraries to have global setup and teardown that can be guaranteed to run in the same context.

Jasmine won't be changing the ordering of afterEach for the foreseeable future.

@jaalzateolaya
Copy link

I think you should document this behaviour. Introduction.js doesn't say anything about it.

@slackersoft
Copy link
Member

@alexander-alzate The source for the Jasmine documentation has its own github repository. We do accept pull requests for our documentation, if you'd like to help out, or issues if you have things you'd like see addressed but aren't sure how.

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.

None yet

8 participants