-
Notifications
You must be signed in to change notification settings - Fork 27.4k
chore: properly isolate module tests #16712
Conversation
angularFiles.js
Outdated
'build/angular.js', | ||
'@angularSrcModules', | ||
'src/ngMock/*.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be here? Can't it shadow problems as before (e.g. modules depending on stuff that is defined in ngMock
's source code)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the angular-mocks file defines the module fn that integrates with jasmine, so I think it unfortunately must always be included:
angular.js/src/ngMock/angular-mocks.js
Line 3085 in bb5a7e3
var module = window.module = angular.mock.module = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we get the same integration with build/angular-mocks.js
, right? (Without the problem of making things defined in the ngMocks
closure available to other modules.)
I would expect build/angular-mocks.js
to be included in all module tests, except for ngMocks
specs, which should indeed use src/ngMock/*.js
. Do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using build/angular-mocks.js
works great, good suggestion!
In the beginning, it was a big hassle to separate all these files, it's possible that at some point the ngMock src files had to be included.
|
||
'karmaModules-ngMessages': [ | ||
'@karmaModulesBase', | ||
'build/angular-animate.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... 🤔
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests in ngMessages are dependent on ngAnimate, e.g.
angular.js/test/ngMessages/messagesSpec.js
Line 465 in bb5a7e3
module('ngAnimate'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need ngAnimate
if we use ngAnimateMock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically yes, but I'm more comfortable with testing the actual animations instead of just the base provided by ng.
angularFiles.js
Outdated
|
||
'karmaModules-ngMock': [ | ||
'@karmaModulesBase', | ||
'build/angular-resource.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... 🤔
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a test that uses ngResource:
angular.js/test/ngMock/angular-mocksSpec.js
Line 1114 in bb5a7e3
'ngResource', |
I think we could use a custom module here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for using a custom module.
607e3d1
to
86d916a
Compare
711f06a
to
2f1f6d9
Compare
2f1f6d9
to
85747ae
Compare
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
Previously, our module tests weren't running in isolation. Theroetically, we wanted to test if the angular.js build file works well with the individual module source files (Note that we don't use the module build files because we sometimes test non-public apis).
However, we bundled all module files together with the build file, and this messed up the results as some modules assign angular helpers like isDefined in the current scope, which means all following modules have access to them too, which essentially defeats the purpose of these tests. See #16702.
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements
Other information: