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

Speed up tests and other perf improvements #381

Merged
merged 20 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
236 changes: 161 additions & 75 deletions dist/autofill-debug.js

Large diffs are not rendered by default.

236 changes: 161 additions & 75 deletions dist/autofill.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module.exports = {
testEnvironment: './jest-test-environment.js',

// Indicates whether each individual test should be reported during the run
verbose: true,
verbose: false,
shakyShane marked this conversation as resolved.
Show resolved Hide resolved

// ensure snapshots are in a JSON format
snapshotFormat: {
Expand Down
16 changes: 16 additions & 0 deletions jest.setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,19 @@ Object.defineProperty(window.HTMLElement.prototype, 'offsetHeight', {
return this._jsdomMockOffsetHeight || 0
}
})

shakyShane marked this conversation as resolved.
Show resolved Hide resolved
// getComputedStyle is super slow on jsdom, by providing this mock we speed tests up significantly
const defaultStyle = {
display: 'block',
visibility: 'visible',
opacity: '1',
paddingRight: '10'
}
const mockGetComputedStyle = (el) => {
return {
// since we don't load stylesheets in the tests, the style prop is all the css applied, so it's a safe fallback
getPropertyValue: (prop) => el.style?.[prop] || defaultStyle[prop]
}
}
// @ts-ignore
jest.spyOn(window, 'getComputedStyle').mockImplementation(mockGetComputedStyle)
7 changes: 1 addition & 6 deletions src/DeviceInterface/ExtensionInterface.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,8 @@ class ExtensionInterface extends InterfacePrototype {
return null
}

removeAutofillUIFromPage () {
super.removeAutofillUIFromPage()
this.activeForm?.removeAllDecorations()
Copy link
Member Author

@GioSensation GioSensation Sep 12, 2023

Choose a reason for hiding this comment

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

I centralized removing all decoration and everything else, so we don't need this override anymore. Find it in the scanner changes below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

love it.

}

async resetAutofillUI (callback) {
this.removeAutofillUIFromPage()
this.removeAutofillUIFromPage('Resetting autofill.')

await this.setupAutofill()

Expand Down
11 changes: 7 additions & 4 deletions src/DeviceInterface/InterfacePrototype.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class InterfacePrototype {
/** @type {boolean} */
isInitializationStarted;

/** @type {(()=>void) | null} */
/** @type {((reason, ...rest) => void) | null} */
_scannerCleanup = null

/**
Expand Down Expand Up @@ -102,9 +102,12 @@ class InterfacePrototype {
return new NativeUIController()
}

removeAutofillUIFromPage () {
/**
* @param {string} reason
*/
removeAutofillUIFromPage (reason) {
this.uiController?.destroy()
this._scannerCleanup?.()
this._scannerCleanup?.(reason)
}

get hasLocalAddresses () {
Expand Down Expand Up @@ -332,7 +335,7 @@ class InterfacePrototype {
postInit () {
const cleanup = this.scanner.init()
this.addLogoutListener(() => {
cleanup()
cleanup('Logged out')
if (this.globalConfig.isDDGDomain) {
notifyWebApp({ deviceSignedIn: {value: false} })
}
Expand Down
5 changes: 5 additions & 0 deletions src/Form/Form.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ class Form {
get isHybrid () {
return this.formAnalyzer.isHybrid
}
get isCCForm () {
return this.formAnalyzer.isCCForm()
}
Copy link
Member Author

@GioSensation GioSensation Sep 12, 2023

Choose a reason for hiding this comment

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

The isCCForm was in the matching class, and was being run on every single field. FormAnalyzer is a more fitting place and allows us to run it once per form, rather than once per field. Massive performance improvement on pages with tons of fields.

Copy link
Collaborator

Choose a reason for hiding this comment

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

excellent


logFormInfo () {
if (!shouldLog()) return
Expand Down Expand Up @@ -352,6 +355,7 @@ class Form {
destroy () {
this.removeAllDecorations()
this.removeTooltip()
this.forgetAllInputs()
shakyShane marked this conversation as resolved.
Show resolved Hide resolved
this.mutObs.disconnect()
this.matching.clear()
this.intObs = null
Expand Down Expand Up @@ -451,6 +455,7 @@ class Form {
const opts = {
isLogin: this.isLogin,
isHybrid: this.isHybrid,
isCCForm: this.isCCForm,
hasCredentials: Boolean(this.device.settings.availableInputTypes.credentials?.username),
supportsIdentitiesAutofill: this.device.settings.featureToggles.inputType_identities
}
Expand Down
39 changes: 39 additions & 0 deletions src/Form/FormAnalyzer.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,45 @@ class FormAnalyzer {
}
return this
}

/** @type {undefined|boolean} */
_isCCForm = undefined
/**
* Tries to infer if it's a credit card form
* @returns {boolean}
*/
isCCForm () {
if (this._isCCForm !== undefined) return this._isCCForm
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only change to this method, apart from being moved here. Just memoizing the result. When the form needs to be rescanned it follows the same flow as isLogin and isSignup. The analyzer is destroyed and recreated, so form data is kept up to date when needed and only when needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

very nice!


const formEl = this.form
const ccFieldSelector = this.matching.joinCssSelectors('cc')
if (!ccFieldSelector) {
this._isCCForm = false
return this._isCCForm
}
const hasCCSelectorChild = formEl.matches(ccFieldSelector) || formEl.querySelector(ccFieldSelector)
// If the form contains one of the specific selectors, we have high confidence
if (hasCCSelectorChild) {
this._isCCForm = true
return this._isCCForm
}

// Read form attributes to find a signal
const hasCCAttribute = [...formEl.attributes].some(({name, value}) =>
/(credit|payment).?card/i.test(`${name}=${value}`)
)
if (hasCCAttribute) {
this._isCCForm = true
return this._isCCForm
}

// Match form textContent against common cc fields (includes hidden labels)
const textMatches = formEl.textContent?.match(/(credit|payment).?card(.?number)?|ccv|security.?code|cvv|cvc|csc/ig)

// We check for more than one to minimise false positives
this._isCCForm = Boolean(textMatches && textMatches.length > 1)
return this._isCCForm
}
}

export default FormAnalyzer
10 changes: 7 additions & 3 deletions src/Form/input-classifiers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ import {createAvailableInputTypes} from '../../integration-test/helpers/utils.js
* @type {object[]}
*/
const testCases = JSON.parse(fs.readFileSync(path.join(__dirname, 'test-cases/index.json')).toString('utf-8'))
testCases.forEach(testCase => {
testCase.testContent = fs.readFileSync(path.resolve(__dirname, './test-cases', testCase.html), 'utf-8')
})
shakyShane marked this conversation as resolved.
Show resolved Hide resolved

/**
* @param {HTMLInputElement} el
Expand Down Expand Up @@ -147,15 +150,16 @@ describe.each(testCases)('Test $html fields', (testCase) => {
expectedSubmitFalsePositives = 0,
expectedSubmitFalseNegatives = 0,
title = '__test__',
hasExtraWrappers = true
hasExtraWrappers = true,
testContent
} = testCase

const testTextString = expectedFailures.length > 0
? `should contain ${expectedFailures.length} known failure(s): ${JSON.stringify(expectedFailures)}`
: `should NOT contain failures`

it(testTextString, () => {
const testContent = fs.readFileSync(path.resolve(__dirname, './test-cases', html), 'utf-8')
it.concurrent(testTextString, async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't yield any performance gains for some reason. I've even tried chunking the testCases and running them in parallel, but nothing seemed to matter much 🤷‍♂️. Ran out of time for this scope and the gains are already pretty significant.

document.body.innerHTML = ''

let baseWrapper = document.body

Expand Down
29 changes: 2 additions & 27 deletions src/Form/matching.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ class Matching {

// // For CC forms we run aggressive matches, so we want to make sure we only
// // run them on actual CC forms to avoid false positives and expensive loops
if (this.isCCForm(formEl)) {
if (opts.isCCForm) {
const subtype = this.subtypeFromMatchers('cc', input)
if (subtype && isValidCreditCardSubtype(subtype)) {
return `creditCards.${subtype}`
Expand Down Expand Up @@ -278,6 +278,7 @@ class Matching {
* @typedef {{
* isLogin?: boolean,
* isHybrid?: boolean,
* isCCForm?: boolean,
* hasCredentials?: boolean,
* supportsIdentitiesAutofill?: boolean
* }} SetInputTypeOpts
Expand Down Expand Up @@ -559,32 +560,6 @@ class Matching {
this.setActiveElementStrings(input, form)
return this
}
/**
* Tries to infer if it's a credit card form
* @param {HTMLElement} formEl
* @returns {boolean}
*/
isCCForm (formEl) {
const ccFieldSelector = this.joinCssSelectors('cc')
if (!ccFieldSelector) {
return false
}
const hasCCSelectorChild = formEl.matches(ccFieldSelector) || formEl.querySelector(ccFieldSelector)
// If the form contains one of the specific selectors, we have high confidence
if (hasCCSelectorChild) return true

// Read form attributes to find a signal
const hasCCAttribute = [...formEl.attributes].some(({name, value}) =>
/(credit|payment).?card/i.test(`${name}=${value}`)
)
if (hasCCAttribute) return true

// Match form textContent against common cc fields (includes hidden labels)
const textMatches = formEl.textContent?.match(/(credit|payment).?card(.?number)?|ccv|security.?code|cvv|cvc|csc/ig)

// We check for more than one to minimise false positives
return Boolean(textMatches && textMatches.length > 1)
}

/**
* @type {MatchingConfiguration}
Expand Down
18 changes: 15 additions & 3 deletions src/Form/matching.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,21 @@ describe('matching', () => {
{ html: `<input placeholder="captcha-password" />`, subtype: 'unknown' },
{ html: `<input placeholder="username" />`, subtype: 'credentials.username' },
{ html: `<input name="username-search" />`, subtype: 'unknown' },
{ html: `<input name="cc-name" />`, subtype: 'creditCards.cardName' },
{ html: `<input name="accountholdername" /><!-- second input is to trigger cc type --><input name="cc-number"/>`, subtype: 'creditCards.cardName' },
{ html: `<input name="Срок действия карты" /><!-- second input is to trigger cc type --><input name="cc-number"/>`, subtype: 'creditCards.expirationMonth' },
{
html: `<input name="cc-name" />`,
subtype: 'creditCards.cardName',
opts: {isCCForm: true}
},
{
html: `<input name="accountholdername" /><!-- second input is to trigger cc type --><input name="cc-number"/>`,
subtype: 'creditCards.cardName',
opts: {isCCForm: true}
},
{
html: `<input name="Срок действия карты" /><!-- second input is to trigger cc type --><input name="cc-number"/>`,
subtype: 'creditCards.expirationMonth',
opts: {isCCForm: true}
},
{ html: `<input placeholder="ZIP code" autocomplete="shipping postal-code" type="text" name="checkout[shipping_address][zip]"/>`, subtype: 'identities.addressPostalCode' },
{ html: `<input autocomplete="on" id="address_line2" name="address_line2" type="text">`, subtype: 'identities.addressStreet2' },
{ html: `<input name="ADDRESS_LINE_1" type="text" aria-required="true" aria-describedby="ariaId_29" aria-labelledby="ariaId_30" autocomplete="off-street-address" aria-autocomplete="list">`, subtype: 'identities.addressStreet' },
Expand Down
2 changes: 1 addition & 1 deletion src/InContextSignup.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export class InContextSignup {

onIncontextSignupDismissed (options = { shouldHideTooltip: true }) {
if (options.shouldHideTooltip) {
this.device.removeAutofillUIFromPage()
this.device.removeAutofillUIFromPage('Email Protection in-context signup dismissed.')
this.device.deviceApi.notify(new CloseAutofillParentCall(null))
}
this.permanentlyDismissedAt = new Date().getTime()
Expand Down
Loading
Loading