Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

chore: properly isolate module tests #16712

Merged
merged 1 commit into from
Oct 15, 2018

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Oct 5, 2018

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

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

angularFiles.js Outdated
'build/angular.js',
'@angularSrcModules',
'src/ngMock/*.js',
Copy link
Member

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)?

Copy link
Contributor Author

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:

var module = window.module = angular.mock.module = function() {

Copy link
Member

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?

Copy link
Contributor Author

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',
Copy link
Member

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?

Copy link
Contributor Author

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.

module('ngAnimate');

Copy link
Member

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?

Copy link
Contributor Author

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',
Copy link
Member

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?

Copy link
Contributor Author

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:


I think we could use a custom module here instead.

Copy link
Member

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.

@Narretz Narretz force-pushed the chore-isolate-modules branch from 607e3d1 to 86d916a Compare October 8, 2018 08:10
@Narretz Narretz force-pushed the chore-isolate-modules branch from 711f06a to 2f1f6d9 Compare October 15, 2018 08:45
@Narretz Narretz force-pushed the chore-isolate-modules branch from 2f1f6d9 to 85747ae Compare October 15, 2018 11:16
@Narretz Narretz merged commit 3930720 into angular:master Oct 15, 2018
@Narretz Narretz deleted the chore-isolate-modules branch October 15, 2018 13:10
Narretz added a commit that referenced this pull request Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants