Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

cironunes
Copy link
Member

I am working on issue #896. It still needs a lot of improvements and even tests. I really appreciate any suggestions.

@@ -0,0 +1,30 @@
var tests = [];
for (var file in window.__karma__.files) {
if (window.__karma__.files.hasOwnProperty(file)) {
Copy link
Contributor

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...

Copy link
Member Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

var pathToModule = function(path) {
return path.replace(/^\/base\//, '').replace(/\.js$/, '');
};

Yeah, we should update the docs too...

@vojtajina
Copy link
Contributor

This looks great!

Currently karma init asks you "do you wanna use require?". If you say so, then it asks two questions about "where are your source/test files" (these files won't be included) and "where is your test-main.js?" (these will be included). I think instead of the second question, it should ask "do you wanna generate test-main.js/coffee?" If you say so, it generates test-main.js and not even ask the "include" question. If you say no, then it should ask the "include question".

What do you think?

@cironunes
Copy link
Member Author

Thanks for the feedback @vojtajina!

I really liked it! I'm going to implement it today later and come back to discuss more.

@cironunes
Copy link
Member Author

I just put the questions that way you said. Now I'm trying to figure out how to test it.

@cironunes
Copy link
Member Author

@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(
Copy link
Contributor

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?

@vojtajina
Copy link
Contributor

@cironunes this is really good! I sent some more comments.

Can you also update the RequireJS docs to mention karma init? Because now, with your update, the whole thing will be much simpler.

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 vojtajina added this to the v0.12 milestone Mar 9, 2014
@vojtajina vojtajina self-assigned this Mar 9, 2014
@cironunes
Copy link
Member Author

@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

@cironunes
Copy link
Member Author

Missing two things that I still didn't figure out:

  • Add the test-main.js for last
  • Unit tests

@vojtajina
Copy link
Contributor

Awesome. Do you need any help?

@cironunes
Copy link
Member Author

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.

@vojtajina
Copy link
Contributor

Wait, you already did the "add test-main.js", right?
I think this is good! I will just squash it into a single commit and merging...

@cironunes
Copy link
Member Author

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...

@vojtajina
Copy link
Contributor

All right, I did subtle changes, fixed the tests, made regexp more flexible... here is my changes vojtajina@50df045

@vojtajina
Copy link
Contributor

Ok, squashed and merged as 85900c9 and a99c387

Thanks a lot @cironunes !!!

@vojtajina vojtajina closed this Mar 10, 2014
@cironunes cironunes deleted the requirejs/test-main branch March 10, 2014 23:45
@cironunes
Copy link
Member Author

Pretty nice!

You're welcome and thank you for Karma! 😄

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

Successfully merging this pull request may close these issues.

None yet

2 participants