Refactor gr-router functions into more testable component methods

Formerly, the most complex of gr-router's logic lived in functions
defined in the closure's scope rather than as methods on the component.
With this change, these functions are refactored into component methods
while preserving their semantics. This opens the door for dramatic
increases in test coverage in upcoming changes.

Summary of refactorings in this change:
- `startRouter` is refactored into a method rather than a plain function
  defined in the closure but is invoked in much the same manner. Whereas
  it formerly accepted the instance-scoped `_generateUrl` method as a
  parameter, it can now access the method via `this` and the parameter
  is removed.
- `restAPI` is promoted to be a Polymer property rather than a local to
  `startRouter`. In upcoming changes, this should be refactored to be
  defined in the usual way and accessed via `this.$`.
- `normalizeLegacyRouteParams` is refactored into a method and its
  behavior is encoded in unit tests.
- `redirectToLogin` is refactored into a method.
- `normalizePatchRangeParams` is refactored into an instance method and
  its behavior is partially encoded in unit tests. Whereas the method
  was formerly untested, one code path is left untested as it revealed
  a bug in the method, and will be patched in an upcoming change.
- Declaring new `app.params` now goes through the new (easily stub-able)
  `_setParams` method.
- Invoking a redirection now goes through the new (also easily
  stub-able) `_redirect` method.
- Behavior methods which were (somewhat hackily) invoked from their
  definitions inside the closure are now used like normal behavior
  methods.
- Tests are added for `_getPatchRangeExpression`.

This change increases gr-router test coverage from 18.9% to 26.4%.
Greater coverage increases are to come in future changes.

Change-Id: I86cb722e218a6ff599d7312d7efd71113f8d5ab4
This commit is contained in:
Wyatt Allen
2017-08-24 10:53:05 -07:00
parent f14482cbb5
commit 84505ea819
2 changed files with 667 additions and 558 deletions

File diff suppressed because it is too large Load Diff

View File

@@ -33,13 +33,17 @@ limitations under the License.
<script>
suite('gr-router tests', () => {
let element;
let sandbox;
setup(() => {
sandbox = sinon.sandbox.create();
element = fixture('basic');
});
teardown(() => { sandbox.restore(); });
suite('generateUrl', () => {
let element;
setup(() => {
element = fixture('basic');
});
test('search', () => {
let params = {
view: Gerrit.Nav.View.SEARCH,
@@ -109,6 +113,88 @@ limitations under the License.
assert.equal(element._generateUrl(params),
'/c/test/+/42/2/file.cpp#b123');
});
test('_getPatchRangeExpression', () => {
const params = {};
let actual = element._getPatchRangeExpression(params);
assert.equal(actual, '');
params.patchNum = 4;
actual = element._getPatchRangeExpression(params);
assert.equal(actual, '4');
params.basePatchNum = 2;
actual = element._getPatchRangeExpression(params);
assert.equal(actual, '2..4');
delete params.patchNum;
actual = element._getPatchRangeExpression(params);
assert.equal(actual, '2..');
});
});
suite('param normalization', () => {
let projectLookupStub;
setup(() => {
projectLookupStub = sandbox
.stub(element._restAPI, 'getFromProjectLookup')
.returns(Promise.resolve('foo/bar'));
sandbox.stub(element, '_generateUrl');
});
suite('_normalizeLegacyRouteParams', () => {
let rangeStub;
setup(() => {
rangeStub = sandbox.stub(element, '_normalizePatchRangeParams')
.returns(Promise.resolve());
});
test('w/o changeNum', () => {
const params = {};
return element._normalizeLegacyRouteParams(params).then(() => {
assert.isFalse(projectLookupStub.called);
assert.isFalse(rangeStub.called);
assert.isNotOk(params.project);
});
});
test('w/ changeNum', () => {
const params = {changeNum: 1234};
return element._normalizeLegacyRouteParams(params).then(() => {
assert.isTrue(projectLookupStub.called);
assert.isTrue(rangeStub.called);
assert.equal(params.project, 'foo/bar');
});
});
});
suite('_normalizePatchRangeParams', () => {
test('range n..n normalizes to n', () => {
const params = {basePatchNum: 4, patchNum: 4};
const needsRedirect = element._normalizePatchRangeParams(params);
assert.isTrue(needsRedirect);
assert.isNotOk(params.basePatchNum);
assert.equal(params.patchNum, 4);
});
test('range n.. normalizes to n', () => {
const params = {basePatchNum: 4};
const needsRedirect = element._normalizePatchRangeParams(params);
assert.isFalse(needsRedirect);
assert.isNotOk(params.basePatchNum);
assert.equal(params.patchNum, 4);
});
test('range 0..n normalizes to edit..n', () => {
const params = {basePatchNum: 0, patchNum: 4};
const needsRedirect = element._normalizePatchRangeParams(params);
assert.isTrue(needsRedirect);
assert.equal(params.basePatchNum, 'edit');
assert.equal(params.patchNum, 4);
});
});
});
});
</script>