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

Fix #383. Refactored test suite to use bower_components so we catch thes... #384

Merged
merged 2 commits into from Sep 9, 2013

Conversation

ssafejava
Copy link
Collaborator

...e errors by staying on top of dep updates. Added a test in views.js to catch this particular error explicitly.

QUnit update now throws errors upon improper use of asyncTest/start/stop, so I fixed those errors as well.

Removed test/vendor and test/qunit directory as they are no longer needed.

Added test/spec directory.

…hese errors by staying on top of dep updates.

QUnit update now throws errors upon improper use of asyncTest/start/stop, so I fixed those errors as well.

Removed test/vendor and test/qunit directory as they are no longer needed.

Added test/spec directory.
@@ -1,2 +1,4 @@
node_modules
test/report
bower_components
.DS_Store
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW global files ignore can/should be added to your own global .gitignore. That helps only keeping project related ignores in the local .gitignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, will remove & update.

@tbranyen
Copy link
Owner

tbranyen commented Sep 9, 2013

Looks like bower needs to be added to devDependencies.

@SBoudrias
Copy link
Collaborator

devDependencies or peerDependencies? I wonder as it'll probably won't run if not installed globally.

@tbranyen
Copy link
Owner

tbranyen commented Sep 9, 2013

Its only for our needs, devDependencies should work fine. Travis puts everything in your PATH anyways.

@ssafejava
Copy link
Collaborator Author

Yeah, that seems right, bower & grunt both won't run at all even if installed in node_modules, so they should be peerDependencies.

For example, yeoman's package.json

@tbranyen
Copy link
Owner

tbranyen commented Sep 9, 2013

What was the reason for moving tests into test/specs?

@ssafejava
Copy link
Collaborator Author

I thought it was clearer. It isn't immediately obvious that setup.js and configure.js are actually test suites, and I think that putting them in the spec folder makes that simpler to understand. Both structures are common in projects I've worked with; if it's a big deal, I can revert.

@tbranyen
Copy link
Owner

tbranyen commented Sep 9, 2013

Not a big deal, just calling it out because it's not relevant to the fix in issue 383. I'd like @jugglinmike to comment on this before merging.

@jugglinmike
Copy link
Collaborator

I agree that it's better to have the tests themselves in a dedicated directory. It would be nice to see the resolution of #383 in an independent commit, although it sounds like I'm alone on that.

@tbranyen
Copy link
Owner

tbranyen commented Sep 9, 2013

Okay cool, merging.

tbranyen added a commit that referenced this pull request Sep 9, 2013
Fix #383. Refactored test suite to use bower_components so we catch thes...
@tbranyen tbranyen merged commit bd40bca into tbranyen:master Sep 9, 2013
@tbranyen tbranyen deleted the bowerTests branch September 9, 2013 16:09
@tbranyen
Copy link
Owner

tbranyen commented Sep 9, 2013

Thanks @ssafejava!

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