-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(init): generate test-main.(js/coffee) for RequireJS projects #897
Conversation
@@ -0,0 +1,30 @@ | |||
var tests = []; | |||
for (var file in window.__karma__.files) { | |||
if (window.__karma__.files.hasOwnProperty(file)) { |
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.
Normalize the paths to "module" ids (without the *.js suffix, etc.) - otherwise RequireJS treats the test files as "scripts" which causes many troubles...
Same for CoffeeScript version...
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.
I did not got it. Actually I just copied this from the docs. Could you explain?
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.
See
karma/test/e2e/requirejs/main.js
Lines 4 to 6 in 258ecc6
var pathToModule = function(path) { | |
return path.replace(/^\/base\//, '').replace(/\.js$/, ''); | |
}; |
Yeah, we should update the docs too...
This looks great! Currently What do you think? |
Thanks for the feedback @vojtajina! I really liked it! I'm going to implement it today later and come back to discuss more. |
I just put the questions that way you said. Now I'm trying to figure out how to test it. |
@vojtajina, just updated here, can you take a look? I updated the docs also. 😄 |
@@ -239,12 +249,23 @@ exports.init = function(config) { | |||
sm.process(questions, function(answers) { | |||
var cwd = process.cwd(); | |||
var configFile = config.configFile || 'karma.conf.js'; | |||
var requirejsConfigFile = path.resolve( |
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.
I think we should generate the test-main.js
file in the same folder as the karma.conf.js
, rather than cwd.
What do you think?
@cironunes this is really good! I sent some more comments. Can you also update the RequireJS docs to mention I'm about to push 0.12 release and I would like to include this in, so let me know if you have time to work on it in the next couple of days, otherwise I'm happy to finish it. Thanks a lot for this! |
@vojtajina cool! I appreciate your feedback and I'm going to work on it. I just updated the docs and the question + hint about the config file. FYI I'm planning to squash commits before merge |
- use the same path of karma.conf.js - include the file
Missing two things that I still didn't figure out:
|
Awesome. Do you need any help? |
Yes, any help would be nice! Actually if you want to finish it would be good too. But if you want to just guide me, I can do that. |
Wait, you already did the "add test-main.js", right? |
I already did, but it should be added for last, right? And the tests are failing. Actually I'm not sure if I wrote reasonable tests for that... |
All right, I did subtle changes, fixed the tests, made regexp more flexible... here is my changes vojtajina@50df045 |
Ok, squashed and merged as 85900c9 and a99c387 Thanks a lot @cironunes !!! |
Pretty nice! You're welcome and thank you for Karma! 😄 |
I am working on issue #896. It still needs a lot of improvements and even tests. I really appreciate any suggestions.