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

reorganize and refactor application, and childView unit tests #3525

Merged

Conversation

wesmangum
Copy link
Contributor

@wesmangum wesmangum commented Nov 6, 2017

Proposed changes

  • Reorganize tests to match order of codebase
  • Import dependencies to move away from using global ones
  • Use let in favor of this in beforeEach

Link to the issue: #3248

This pr is a small part of the work to refactor the whole unit tests directory. Merging this issue should keep #3248 open

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7c85c12 on wesmangum:tests/app-behavior-childView into 08a5c7f on marionettejs:next.

@@ -36,22 +37,6 @@ describe('Marionette Application', function() {
});
});

describe('#start', function() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this could move back. start triggers onBeforeStart so it's first in the stack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems fair, just tried to match the order of the codebase file.

Copy link
Member

Choose a reason for hiding this comment

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

well yes but:

1.  start(options) {
2.   this.triggerMethod('before:start', this, options);  //onBeforeStart
3.   this.triggerMethod('start', this, options);  //onStart

@@ -1,14 +1,19 @@
import Marionette from '../../src/backbone.marionette';
Copy link
Member

Choose a reason for hiding this comment

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

let's import Behavior directly rather than Marionette

@paulfalgout
Copy link
Member

Could we address the revision requests and conflicts and leave the view test changes to another PR?

@wesmangum
Copy link
Contributor Author

@paulfalgout My apologies, I was planning to create another branch for the views work. I'll revert that now

@wesmangum wesmangum force-pushed the tests/app-behavior-childView branch 2 times, most recently from 7c85c12 to e3e3311 Compare November 9, 2017 16:28
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4487297 on wesmangum:tests/app-behavior-childView into 2b33e64 on marionettejs:next.

@paulfalgout paulfalgout changed the title reorganize and refactor application, behavior, and childView unit tests reorganize and refactor application, and childView unit tests Nov 9, 2017
@paulfalgout
Copy link
Member

No source changes so I'm merging. Can fix it if anyone objects in subsequent PRs

@paulfalgout paulfalgout merged commit 91ab7ea into marionettejs:next Nov 9, 2017
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.

3 participants