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

Clean up tests #3248

Open
paulfalgout opened this issue Nov 2, 2016 · 10 comments
Open

Clean up tests #3248

paulfalgout opened this issue Nov 2, 2016 · 10 comments

Comments

@paulfalgout
Copy link
Member

Looking over the tests many of them use older terms (some even Marionette v1). There are a few tests that were modified to match the current behavior, but aren't in fact testing anything anymore. And in general the tests could be updated to be cleaner and more consistent.

Further I believe most of the tests should be organized by class and then by either a feature of that class or an integration with a mixin or other class. We have too many one-off test files that it is difficult to tell where to add something.

I'll be playing with organization and style of tests while writing new ones for #3244 while examining what is currently there. I just want to be thinking about what improvements can be made.

Additionally there's a Readme file in the test directory that is completely irrelevant to our current setup. 😞

@denar90 denar90 self-assigned this Nov 2, 2016
@paulfalgout
Copy link
Member Author

As mentioned above 5603b0e

@rafde
Copy link
Member

rafde commented Nov 8, 2016

I agree with this, I think a clean up outline is in order.
@denar90 is tackling Application
There are so many test, I don't know where to start.

@denar90
Copy link
Member

denar90 commented Nov 8, 2016

@rafde moreover, I'm working with behaviors also.

@paulfalgout
Copy link
Member Author

I think the best way to approach this work might be to start testing the things with the least amount of dependencies. So I'd think we could make a utils directory (just have the test folder structure mirror the src) and make a unit test for each file... most likely by pulling from and updating older tests.. I think we can possibly ignore CollectionView, CompositeView, and AppRouter stuff for now.
We definitely won't want to get rid of anything until we know there's a test that covers the same case.

@denar90
Copy link
Member

denar90 commented Nov 10, 2016

Sounds really good 👍 . It will be easy to find test file if it be a mirror of src folder. In addition, starting with behaviors was just experiment how it can goes, importing stuff from src etc.

@paulfalgout
Copy link
Member Author

I think for large classes with lots of integrations we can break up the tests into multiple files, but they can be put in a folder that matches the file name. live view/view.spec.js view/view-ui.spec.js view/view-behaviors.spec.js etc.

@rafde
Copy link
Member

rafde commented Nov 10, 2016

Some of the specs could probably be renamed also, like https://github.com/rafde/backbone.marionette/blob/e887d160ea8df451f36bd8474b17edd59648b14e/test/unit/view.triggers.spec.js#L1-L1 is actually testing from view.$el.trigger

@denar90
Copy link
Member

denar90 commented Nov 10, 2016

SGTM

@paulfalgout
Copy link
Member Author

Maybe we should make a project for this? I've never tried it.

@denar90
Copy link
Member

denar90 commented Feb 16, 2017

I tried to do that
here we are https://github.com/marionettejs/backbone.marionette/projects/1 🎉

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

No branches or pull requests

3 participants