Jasmine 2.1 beforeAll/afterAll methods cause errors #10238
Comments
|
I posted this bug on Jasmine as well because I wasn't sure which code was in the wrong: jasmine/jasmine#714 They replied with this:
|
|
@chesleybrown it seems to me that you are trying to do something like: beforeAll(inject(function ($rootScope) {
}));If this is the case, then this is the expected behavior since angular needs to create a new injector in each test. I don't think we want to be able to have a single injector for all the tests in a suite. I guess it can be done, but I don't think we should unless you have a good use case for this... (of course I'm just assuming that this is what you did from looking at the stack trace and it is possible you did something else entirely :D) |
|
After two months and no feedback, I made the assumption that the issue is what @shahata commented. Closing this, but if there is anything else that needs to be handled, then feel free to reopen the issue |
|
The use case I have for using beforeAll looks something like this: I like being able to split up the assertions into more granular chunks instead of just listing them one after another in a single it block. I think this is still achievable with beforeEach (just initialize object if it's null), it's just not quite so straightforward. |
|
So... is the bottom line that Angular doesn't support Jasmine's |
|
It is not possible to use the injector at |
|
@lgalfaso not sure why this would be an Jasmine issue? Jasmine's As @xml said, seems a shame to talk about Jasmine as the most popular framework in the docs, and not support a very core feature for writing good tests in the style @scriby demonstrated. |
|
@timruffles each test must have its own clean Loose comments stating this is a shame do not help. The project karma-jasmine adds some functionality to Jasmine to make it possible for it to be used within karma to test Angular apps. My comment above does not claim that there is a need to change how Jasmine works, it is a great library that works in the expected way. That does not mean that all the functionality provided by Jasmine can be used with the functionality provided by karma-jasmine. One of the limitation is that you do not have access to |
|
@lgalfaso sometimes code is a better way to explain. I'm a GDE so I'd be happy to get started with contribution by productionizing this implementation, rather than just doing talks :) Here is a repo which demonstrates how it is possible to support this functional changing only ngMock. This demonstrates it's ngMock's jasmine/mocha support that is the limiting factor here. Rather than tying injector state to As you can see, it supports beforeEach/beforeAll and the standard use of injectors. |
|
@timruffles can you make your variation of ngMock work with this simple currently working example describe('something', function() {
beforeEach(module(function($interpolateProvider) {
$interpolateProvider.startSymbol('[[');
$interpolateProvider.endSymbol(']]');
}));
it('should interpolate using [[ ]]', inject(function($interpolate) {
expect($interpolate('[[foo]]')({foo: 'bar'})).toBe('bar');
}));
});
describe('something else', function() {
it('should interpolate using {{ }}', inject(function($interpolate) {
expect($interpolate('{{foo}}')({foo: 'bar'})).toBe('bar');
}));
}); |
|
Thanks for the example, yes I can! I added opt in so you can selectively turn on the new behaviour for given tests. Your example passes without modification, I updated my test with the opt in Run |
|
Good, I think we are going somewhere, but at the end the main point is not clear.
This is, the following code should work beforeAll(inject(function($http) {}));
// Paste here the previous example code |
|
Great.
I'm not sure what the intention is in supporting describe('some expensive to create object', function() {
var object;
beforeAll(module("someModule"))
beforeAll(inject(function(createExpensiveObject) {
object = createExpensiveObject();
}));
it('has property A', function() {
expect(object).toHavePropertyA();
});
it('has property B', function() {
expect(object).toHavePropertyB();
});
});[1] e.g, beforeAll(inject(function($http) {}));
describe('something', function() {
beforeEach(module(function($interpolateProvider) {
$interpolateProvider.startSymbol('[[');
$interpolateProvider.endSymbol(']]');
}));
it('should interpolate using [[ ]]', inject(function($interpolate) {
expect($interpolate('[[foo]]')({foo: 'bar'})).toBe('bar');
}));
});
describe('something else', function() {
it('should interpolate using {{ }}', inject(function($interpolate) {
expect($interpolate('{{foo}}')({foo: 'bar'})).toBe('bar');
}));
});throws this with the current |
|
In case you're reading in email, I edited the comment above with an example of the use cases for |
|
@lgalfaso I'm up for writing this up into a PR if we're agreed on beforeAll support via opt-in. |
|
I was not able look at your approach, should have some time tomorrow. That On Saturday, October 31, 2015, Tim Ruffles [email protected] wrote:
|
|
@timruffles I am reading your comment on the proposal and I am not sure how you are proposing this to work. Let me write down what my understanding on your proposal is, and if this is not correct, please write the comprehensive idea. When there is a Eg. describe('X', function() {
// Here, each test has its own $injector.
it('X1', inject(function(...) {}));
it('X2', inject(function(...) {}));
it('X3', inject(function(...) {}));
});
describe('Y', function() {
// Here, all test share the same $injector.
beforeAll(inject(function(...) {}));
it('Y1', inject(function(...) {}));
it('Y2', inject(function(...) {}));
it('Y3', inject(function(...) {}));
}); |
|
Really it's not tied to The implementation is exposing a
A complete use-case would be: describe('some expensive to create object', function() {
var object;
beforeAll(module.enableManualInjector)
beforeAll(module("someModule"))
afterAll(module.disableManualInjector)
beforeAll(inject(function(createExpensiveObject) {
object = createExpensiveObject();
}));
it('has property A', function() {
expect(object).toHavePropertyA();
});
it('has property B', function() {
expect(object).toHavePropertyB();
});
}); |
|
@timruffles I think this conversation is not moving forward. Exposing a test configuration that has global side-effects and that depends on the good will of developers to undo this configuration, is a bad practice. Test should be isolated and independent by default and must be impossible for some configuration in another file, left behind by some sloppy developer or just because some other test did (not) run before to affect the stability or result of any other test that did not explicitly opted-in. Now, there is a need to allow Any proposal must follow these constraints. If you are willing to work on a PR within these constraints, then I am ok in continuing with this discussion. If you still think that following these constraints are not what you have in mind, then I would feel more comfortable with some other core member team to continue on my behalf. |
|
Great points. I've updated the solution to expose only one method: That avoids the danger you mentioned where other tests would be affected, and doesn't require developers to manually cleanup. Please let me know if there are other constraints I've missed. Otherwise I'll take a look at that PR. describe('allowing shared injectors, without affecting other tests', function() {
describe('tests with sharedInjector', function() {
// within here, we've got 1 injector across all beforeEach, tests etc
module.sharedInjector();
beforeAll(module("main", function($provide) {
$provide.value("stubMe", {
service: "stubbed",
})
}))
})
describe('tests with isolation', function() {
// within here, we've got isolated injectors again
beforeEach(module("main", function($provide) {
$provide.value("stubMe", {
service: "stubbed",
})
}))
})
}); |
|
@lgalfaso hopefully I've addressed your points above? Was just reminded of this work by this related issue on karma-mocha being closed. |
|
@timruffles sorry for taking this long to answer, I know I still have to look into this. At the API level, I think this is fine, but did not have the time to look at the code/test/docs. Hopefully will have some time next week |
|
Hello @timruffles and @lgalfaso - I am late to this discussion but have just popped in. |
|
It would still be a big surprize if all of a sudden my |
|
Good point @gkalpak, but as Very clear documentation is absolutely important tho! I'll update the docs at the same time in the PR. |
|
@gkalpak it would only be all of a sudden if you actually had a |
|
"all of a sudden" was not in the sense of a BC, but more like:
Banning the use of All I'm saying is that going from "1 injector per block" to "1 injector for all blocks" is not explicitly clear just because I used a different method and it should be clearly documented (be it one the dev guide, the changelog or wherever appropriate). |
|
@gkalpak Do you have any suggestions to improve the situation? |
|
All I am saying is, if we will have it work automagically (i.e. without the user's having to opt-in with something like the That's all 😃 |
|
How about an explicit API: |
|
It sounds pretty good to me. (But you still have to document it - there's no escaping that 😛) |
|
There are decisions to be made about interacting with non-shared injector code. I think for safety it's best to disallow this: This could technically work, as we'd see the following calls: However, I imagine a lot of people will get into nasty situations where their So for safety I think we should only support As desiring a shared injector is probably the exception rather than the rule, this restriction seems ok! If this looks ok with you @petebacondarwin, I'll rebase my work into a PR. |
|
Just ran into this problem. Was this issue resolved with v1.6.1? |
|
See the docs here on how to use |
|
@timruffles Thanks! |
|
(This has been fixed since v1.5.1 btw.) |
Jasmine has released
beforeAllandafterAllmethods with 2.1 which allow executing abeforefunction only once for all the following specs. Angular mocks doesn't seem to play nice with this concept.When using
beforeAllorafterAllmethods, I encounter the following errors.The text was updated successfully, but these errors were encountered: