for #26: refactor: new getRequestEntity function #104
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,3 +12,6 @@ before_install: | |
| script: | ||
| - npm run build | ||
| - npm run lint | ||
|
|
||
| after_success: | ||
| - npm run coveralls | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,40 @@ | ||
| const {log} = require('./log') | ||
| const {hostInEntity} = require('./lists') | ||
|
|
||
| function requestAllower (tabID, totalExecTime, startDateTime) { | ||
| totalExecTime[tabID] += Date.now() - startDateTime | ||
| return {} | ||
| } | ||
|
|
||
| function getRequestEntity (entityList, originTopHost, requestTopHost, mainFrameOriginTopHost) { | ||
| let requestEntityName = null | ||
| let sameEntity = false | ||
| for (let entityName in entityList) { | ||
| let entity = entityList[entityName] | ||
| let requestIsEntityResource = false | ||
| let originIsEntityProperty = false | ||
| let mainFrameOriginIsEntityProperty = false | ||
|
|
||
| requestIsEntityResource = hostInEntity(entity.resources, requestTopHost) | ||
| if (requestIsEntityResource) { | ||
| requestEntityName = entityName | ||
|
There was a problem hiding this comment. @groovecoder it seems that Other questions that come to my mind currently are:
There was a problem hiding this comment. Great catch! Yes we can exit the When no match has been found, it needs to return an object with There should not be multiple matches in the I will add tests for these cases. |
||
| } | ||
|
|
||
| originIsEntityProperty = hostInEntity(entity.properties, originTopHost) | ||
|
|
||
| mainFrameOriginIsEntityProperty = hostInEntity(entity.properties, mainFrameOriginTopHost) | ||
|
|
||
| if ((originIsEntityProperty || mainFrameOriginIsEntityProperty) && requestIsEntityResource) { | ||
| log(`originTopHost ${originTopHost} and resource requestTopHost ${requestTopHost} belong to the same entity: ${entityName}; allowing request`) | ||
| sameEntity = true | ||
| break | ||
| } | ||
| } | ||
| // TODO: https://github.com/mozilla/blok/issues/110 | ||
| return {'entityName': requestEntityName, 'sameEntity': sameEntity} | ||
|
There was a problem hiding this comment. Small nit, but you could probably just use the shortcut syntax for return {entityName: requestEntityName, sameEntity} |
||
| } | ||
|
|
||
| module.exports = { | ||
| requestAllower, | ||
| getRequestEntity | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,51 @@ | ||
| var test = require('tape') | ||
| var {requestAllower, getRequestEntity} = require('../js/requests') | ||
|
|
||
| var entityListFixtureData = require('./entitylist-fixture.json') | ||
|
|
||
| test('allowRequest returns {}', (t) => { | ||
| t.plan(1) | ||
| let tabID = 1 | ||
| let totalExecTime = {} | ||
| totalExecTime[tabID] = 0 | ||
| var allowRequest = requestAllower.bind(null, tabID, totalExecTime, Date.now()) | ||
| t.deepEqual(allowRequest(), {}) | ||
| }) | ||
|
|
||
| test('allowRequest adds some ms to totalExecTime[tabID]', (t) => { | ||
| t.plan(1) | ||
| let tabID = 1 | ||
| let totalExecTime = {} | ||
| totalExecTime[tabID] = 0 | ||
| var allowRequest = requestAllower.bind(null, tabID, totalExecTime, 0) | ||
| allowRequest() | ||
| t.ok(totalExecTime[tabID] > 0, 'added ms to totalExecTime[tabID]') | ||
| }) | ||
|
|
||
| test('getRequestEntity request to google.com from facebook.com returns Google and false', (t) => { | ||
|
There was a problem hiding this comment. @groovecoder Like described in the above comment, it could be reasonable to add some additional test case here, e.g. for a request that doesn't have a match in the list loaded from the fixture (and maybe one with a fake host list fixture where the request has a matches in two different "host entities" in the list?) |
||
| t.plan(2) | ||
| let requestEntity = getRequestEntity(entityListFixtureData, 'facebook.com', 'google.com', 'facebook.com') | ||
| t.equal(requestEntity.entityName, 'Google') | ||
| t.notOk(requestEntity.sameEntity) | ||
| }) | ||
|
|
||
| test('getRequestEntity request to facebook.com from instagram.com returns Facebook and true', (t) => { | ||
| t.plan(2) | ||
| let requestEntity = getRequestEntity(entityListFixtureData, 'instagram.com', 'facebook.com', 'instagram.com') | ||
| t.equal(requestEntity.entityName, 'Facebook') | ||
| t.ok(requestEntity.sameEntity) | ||
| }) | ||
|
|
||
| test('getRequestEntity request to facebook.com from github.io iframe on facebook.com returns Facebook and true', (t) => { | ||
| t.plan(2) | ||
| let requestEntity = getRequestEntity(entityListFixtureData, 'githhub.io', 'facebook.com', 'facebook.com') | ||
| t.equal(requestEntity.entityName, 'Facebook') | ||
| t.ok(requestEntity.sameEntity) | ||
| }) | ||
|
|
||
| test('getRequestEntity request to ap.com from tulsaworld.com iframe returns null and false', (t) => { | ||
| t.plan(2) | ||
| let requestEntity = getRequestEntity(entityListFixtureData, 'tulsaworld.com', 'ap.com', 'tulsaworld.com') | ||
| t.equal(requestEntity.entityName, null) | ||
| t.notOk(requestEntity.sameEntity) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of
.indexOf(..) === -1, not sure if we can useArray.includes().Seems like it's maybe supported in Firefox 43+. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I tried that first and got an error ... something to do with some of the elements values being numeric?