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

chore: add build into test unit task #10188

Closed
wants to merge 1 commit into from
Closed

Conversation

Vibhuti
Copy link
Contributor

@Vibhuti Vibhuti commented Nov 22, 2014

While following setup document https://docs.angularjs.org/misc/contribute, and running
$ grunt test:unit, it was showing "Uncaught ReferenceError: angular is not defined - AngularJS not working" error.
So i added 'build' into test unit task and then it starts working.

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@IgorMinar IgorMinar added this to the ng-fixit #1 milestone Nov 24, 2014
@IgorMinar
Copy link
Contributor

@Vibhuti can you please sign CLA (see instructions above)

@Vibhuti
Copy link
Contributor Author

Vibhuti commented Nov 24, 2014

@IgorMinar, i signed CLA.

@googlebot
Copy link

CLAs look good, thanks!

@gkalpak
Copy link
Member

gkalpak commented Nov 24, 2014

Hm...I don't think we need a build step for unit-testing. It will only make the process more time-consuming. Do I miss something ?

@cironunes
Copy link
Member

@gkalpak if you don't run build at least once before running the tests it won't work (I think that this is why @Vibhuti opened the PR, am I right?). However, I don't think that including build before the tests is a good way to solve the issue.

@Vibhuti
Copy link
Contributor Author

Vibhuti commented Nov 24, 2014

@gkalpak you are right. I also agree with the point that we don't need build before running test.
But as @cironunes is saying test will not work if you are not including build.
On the second thought we can include build command in the setup doc https://docs.angularjs.org/misc/contribute. instead of adding it into test unit task?

@gkalpak
Copy link
Member

gkalpak commented Nov 24, 2014

Without actually testing it, the reason a build is needed is because of https://github.com/angular/angular.js/blob/master/angularFiles.js#L175:

"karmaModules": [
    'build/angular.js',
    ...

So, tests:modules uses build/angular.js (which is only generated by during the build task).
I think it should use @angularSrc instead (as do both tests:jqlite and tests:jquery).
So, (according to my untested theory), replacing 'build/angular.js' with '@angularSrc' should fix the issue and stop requiring the build task to have been run before the test task.

Furthermore (but unrelated to this PR), karma-modules.conf.js#L10 unnecessarily requests @angularSrcModules, which is already loaded by karmaModules (angularFiles.js#L176).

@Vibhuti
Copy link
Contributor Author

Vibhuti commented Nov 25, 2014

@gkalpak, i tried to change the angularFiles.js (https://github.com/angular/angular.js/blob/master/angularFiles.js#L175) as per your suggestion, but it's still showing error ( http://pastie.org/9741467) while testing ($ grunt test:unit).

@gkalpak
Copy link
Member

gkalpak commented Nov 25, 2014

@Vibhuti:
Indeed. The modules depend on the angular global object to be fully setup.
So, looking into how we could avoid the need to build.
If nothing else we don't need the whole build task, just build:angular (I think).

@anyone: Any thoughts ?

@cironunes
Copy link
Member

Maybe a git hook to run build:angular on clone?

@gkalpak
Copy link
Member

gkalpak commented Nov 25, 2014

@cironunes: That wouldn't be safe in case you modify angular core, because tests:modules would be running against an outdated build/angular.js file. The build:angular task has to be run every time before tests:modules (unless there is a more efficient (and reliable enough) method of making angular core available to modules; which probably isn't).

After all, I don't think we can circumevent this (having to execute build:angular), but it shouldn't be much of an overhead (building just angular).
IMO, it is best to incorporate it into tests:modules (so we don't have to call it explicitly and risk accidentally omitting it etc), but I am not sure what other implications this might have (if any).

@petebacondarwin
Copy link
Contributor

I tweaked the position of the build dependency and merged. Thansk @Vibhuti

@Vibhuti
Copy link
Contributor Author

Vibhuti commented Jan 25, 2015

Awesome, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants