reorganize and refactor common buildRegion unit tests #3548
Conversation
| this.fooSelector = '#foo-region'; | ||
| this.fooRegion = new this.DefaultRegionClass({el: this.fooSelector}); | ||
| fooSelector = '#foo-region'; | ||
| new DefaultRegionClass({el: fooSelector}); |
There was a problem hiding this comment.
I'm not sure what this line is doing. I suspect the this.fooRegion was removed, but the beforeEach wasn't cleaned up. if we're not using this, there's no reason to keep it around.
| beforeEach(function() { | ||
| this.region = this.view.addRegion(_.uniqueId('region_'),this.BarRegion); | ||
| region = view.addRegion(_.uniqueId('region_'),BarRegion); |
| this.view.addRegion(_.uniqueId('region_'),this.BarRegion); | ||
| }.bind(this); | ||
| buildRegion = function() { | ||
| view.addRegion(_.uniqueId('region_'),BarRegion); |
There was a problem hiding this comment.
same here. linter must not be very strict 😞
| beforeEach(function() { | ||
| this.BarRegion = Marionette.Region.extend(); | ||
| BarRegion = Region.extend(); |
There was a problem hiding this comment.
the redefinition seems unnecessary
There was a problem hiding this comment.
The above assignment of BarRegion has an el assignment, which would make these tests fail. Is there a better way to handle the reassignment?
| beforeEach(function() { | ||
| this.region = this.view.addRegion(_.uniqueId('region_'),this.barRegion); | ||
| region = view.addRegion(_.uniqueId('region_'),barRegion); |
There was a problem hiding this comment.
in this case maybe we can move the instantiation down inside this beforeEach it seems specific to this describe rather than all of them.. in fact this can probably just be , new BarRegion()
| region2Definition = _.defaults({el: el}, baseDefinition); | ||
| region3Definition = _.defaults({el: $el}, baseDefinition); | ||
|
|
||
| new BazRegion({el: fooSelector}); |
| }); | ||
|
|
||
| describe('with `el` also defined', function() { | ||
| let baseDefinition; |
There was a problem hiding this comment.
some of these only need to be lets if they're tested outside of the beforeEach if it's just a variable used within a single function let's keep it const
| }); | ||
| }); | ||
|
|
||
| describe('with `regionClass` defined', function() { | ||
| let $el; |
There was a problem hiding this comment.
these el and $el are only used inside the next describe block.. let's move them there
| this.view.addRegion(_.uniqueId('region_'),this.definition); | ||
| }.bind(this); | ||
| buildRegion = function() { | ||
| view.addRegion(_.uniqueId('region_'),definition); |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('when el is an empty jQuery object', function() { | ||
| let el; | ||
| let definition; | ||
| let buildRegion; |
There was a problem hiding this comment.
as below I think buildRegion is the only necessary let, the others can be const inside the beforeEachs
wesmangum
force-pushed
the
tests/common/buildRegion
branch
2 times, most recently
from
e348b1f
to
831e288
Compare
wesmangum
force-pushed
the
tests/common/buildRegion
branch
from
831e288
to
a4d771d
Compare
Proposed changes
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