From 0575ceb11efbc7f863b615c1404b6fe916645835 Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Sun, 9 Dec 2018 23:12:19 -0500 Subject: [PATCH 1/8] Add failing test case --- tests/overlapping-dependencies.js | 158 ++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 tests/overlapping-dependencies.js diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js new file mode 100644 index 0000000..769cce5 --- /dev/null +++ b/tests/overlapping-dependencies.js @@ -0,0 +1,158 @@ +/* eslint-env mocha */ +/* eslint-disable prefer-arrow-callback */ +import chai from 'chai'; +import filter from 'lodash/filter'; + +import { + COMPARISON_PRESETS, + createParameterizedRootSelector, + createParameterizedSelector, +} from '../src/index'; + +const assert = chai.assert; // eslint-disable-line prefer-destructuring + + +// This is basically an implementation of assert.deepInclude but with better error messages. +// @TODO: Is this useful enough to promote this to its own file, and possibly export? +const assertCountsForParams = (selectorFn, params, expectedCounts) => { + const actualCounts = selectorFn.getAllCountsForParams(params); + + const verboseErrorInfo = `Checking counts for ${selectorFn.displayName}(${params}): + Expected: ${JSON.stringify(expectedCounts)} + Actual: ${JSON.stringify(actualCounts)}\n`; + + Object.keys(expectedCounts).forEach(function (key) { + if (!Object.hasOwnProperty.call(actualCounts, key)) { + assert.fail(`${verboseErrorInfo}Invalid count type for assertCountsForParams: "${key}" not found`); + } + assert.equal(actualCounts[key], expectedCounts[key], `${verboseErrorInfo}Count type "${key}" should match`); + }); +}; + + +describe('Overlapping dependencies', () => { + const initialState = { + appointmentDataById: { + 0: { dayNum: 0, title: 'Start of semester' }, + 1: { dayNum: 1, title: 'Orientation' }, + 2: { dayNum: 5, title: 'Classes begin' }, + 3: { dayNum: 50, title: 'Midterms, day 1' }, + 4: { dayNum: 51, title: 'Midterms, day 2' }, + 5: { dayNum: 95, title: 'Finals, day 1' }, + 6: { dayNum: 96, title: 'Finals, day 2' }, + 7: { dayNum: 97, title: 'Finals, day 3' }, + 8: { dayNum: 99, title: 'End of semester' }, + 20: { dayNum: 10, title: 'Day 10' }, + 21: { dayNum: 20, title: 'Day 20' }, + 22: { dayNum: 30, title: 'Day 30' }, + 23: { dayNum: 40, title: 'Day 40' }, + 24: { dayNum: 50, title: 'Day 50' }, + 25: { dayNum: 60, title: 'Day 60' }, + 26: { dayNum: 70, title: 'Day 70' }, + 27: { dayNum: 80, title: 'Day 80' }, + 28: { dayNum: 90, title: 'Day 90' }, + 40: { dayNum: 15, title: 'Alice\'s birthday' }, + 41: { dayNum: 25, title: 'Bob\'s birthday' }, + 42: { dayNum: 41, title: 'Chris\'s birthday' }, + 43: { dayNum: 89, title: 'Alice & Bob\'s anniversary' }, + 44: { dayNum: 11, title: 'Break' }, + 45: { dayNum: 12, title: 'Break' }, + 46: { dayNum: 13, title: 'Break' }, + 47: { dayNum: 75, title: 'Vacation' }, + 48: { dayNum: 76, title: 'Vacation' }, + 49: { dayNum: 77, title: 'Vacation' }, + }, + }; + const makeDateObjectForDay = dayNum => new Date(2018, 0, dayNum); + + let selectRawAppointmentData; + let selectRawAppointmentIds; + let selectAppointmentById; + let selectAllAppointments; + let selectAppointmentsForDay; + let selectAppointmentsForDayRange; + + beforeEach(() => { + // The selectors get recreated for each test, to reset their call counts. + selectRawAppointmentData = createParameterizedRootSelector( + function _selectRawAppointmentData(state, authorId) { + return state.appointmentDataById[authorId]; + }, + { verboseLoggingEnabled: true } + ); + selectRawAppointmentIds = createParameterizedRootSelector( + function _selectRawAppointmentIds(state) { + return Object.keys(state.appointmentDataById); + }, + { + compareSelectorResults: COMPARISON_PRESETS.SHALLOW_EQUAL, + }, + ); + + // This extra layer represents a transformation or conversion from serializable Redux data into something + // with rich objects, e.g. if you're using a date library. + selectAppointmentById = createParameterizedSelector( + function _selectAppointmentById({ appointmentId }) { + const rawAppointmentData = selectRawAppointmentData(appointmentId); + return { + ...rawAppointmentData, + dateObject: makeDateObjectForDay(rawAppointmentData.dayNum), + }; + }, + { verboseLoggingEnabled: true } + ); + + selectAllAppointments = createParameterizedSelector( + function _selectAllAppointments() { + const rawAppointmentIds = selectRawAppointmentIds(); + return rawAppointmentIds.map( + appointmentId => selectAppointmentById({ appointmentId }), + ); + }, + ); + + selectAppointmentsForDay = createParameterizedSelector( + function _selectAppointmentsForDay({ dayNum }) { + // This naive implementation is going to walk through all appointments + const allAppointments = selectAllAppointments(); + return filter(allAppointments, appointment => appointment.dayNum === dayNum); + }, + ); + selectAppointmentsForDayRange = createParameterizedSelector( + function _selectAppointmentsForDay({ startDayNum, endDayNum }) { + // This naive implementation is going to walk through all appointments + const allAppointments = selectAllAppointments(); + return filter( + allAppointments, + appointment => appointment.dayNum >= startDayNum && appointment.dayNum <= endDayNum, + ); + }, + ); + }); + + // First, some tests to ensure the selectors work as expected in general. + + it('should return appointment models by ID', () => { + const appointment1 = selectAppointmentById(initialState, { appointmentId: 1 }); + const appointment2 = selectAppointmentById(initialState, { appointmentId: 2 }); + + assert.equal(appointment1.title, 'Orientation'); + assert.equal(appointment2.title, 'Classes begin'); + + // No changes after a no-change state change + const newState = { ...initialState }; + assert.equal(appointment1, selectAppointmentById(newState, { appointmentId: 1 })); + assert.equal(appointment2, selectAppointmentById(newState, { appointmentId: 2 })); + + assertCountsForParams(selectRawAppointmentData, 1, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); + assertCountsForParams(selectAppointmentById, { appointmentId: 1 }, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); + }); +}); From 00e370a514070e80f43bcabeecf407c57614cfde Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Sun, 9 Dec 2018 23:22:57 -0500 Subject: [PATCH 2/8] Better logging --- src/defaultOptions.js | 2 ++ src/parameterizedSelectorFactory.js | 19 +++++++++++++++++++ tests/overlapping-dependencies.js | 4 ++-- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/defaultOptions.js b/src/defaultOptions.js index 8d2c27c..0e9f189 100644 --- a/src/defaultOptions.js +++ b/src/defaultOptions.js @@ -38,6 +38,8 @@ const defaultOptions = { // Some options can be changed anytime displayName: null, useConsoleGroup: true, + runLoggingEnabled: false, + runLoggingCallback: console.log, // eslint-disable-line no-console verboseLoggingEnabled: false, verboseLoggingCallback: console.log, // eslint-disable-line no-console performanceChecksEnabled: (typeof __DEV__ !== 'undefined' && !!__DEV__), diff --git a/src/parameterizedSelectorFactory.js b/src/parameterizedSelectorFactory.js index efe2474..55a8fe8 100644 --- a/src/parameterizedSelectorFactory.js +++ b/src/parameterizedSelectorFactory.js @@ -264,6 +264,9 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { previousResult.invokeCount += 1; } } + if (options.runLoggingEnabled) { + console.group(`Invoke: ${loggingPrefix}`); + } if (typeof options.onInvoke === 'function') { options.onInvoke(/* @TODO: What should go here? */); } @@ -335,6 +338,10 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { globalSkippedRunCount += 1; newResult.skippedRunCount += 1; } + if (options.runLoggingEnabled) { + options.runLoggingCallback(`Skipped run: ${loggingPrefix}`); + console.groupEnd(); + } if (typeof options.onSkippedRun === 'function') { options.onSkippedRun(/* @TODO: What should go here? */); } @@ -395,6 +402,10 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { globalPhantomRunCount += 1; newResult.phantomRunCount += 1; } + if (options.runLoggingEnabled) { + options.runLoggingCallback(`Phantom run: ${loggingPrefix}`); + console.groupEnd(); + } if (typeof options.onPhantomRun === 'function') { options.onPhantomRun(/* @TODO: What should go here? */); } @@ -409,6 +420,10 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { globalFullRunCount += 1; newResult.fullRunCount += 1; } + if (options.runLoggingEnabled) { + options.runLoggingCallback(`Full run: ${loggingPrefix}`); + console.groupEnd(); + } if (typeof options.onFullRun === 'function') { options.onFullRun(/* @TODO: What should go here? */); } @@ -449,6 +464,10 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { globalAbortedRunCount += 1; newResult.abortedRunCount += 1; } + if (options.runLoggingEnabled) { + options.runLoggingCallback(`Aborted run: ${loggingPrefix}`); + console.groupEnd(); + } if (typeof options.onAbortedRun === 'function') { options.onAbortedRun(/* @TODO: What should go here? */); } diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js index 769cce5..991a9cd 100644 --- a/tests/overlapping-dependencies.js +++ b/tests/overlapping-dependencies.js @@ -78,7 +78,7 @@ describe('Overlapping dependencies', () => { function _selectRawAppointmentData(state, authorId) { return state.appointmentDataById[authorId]; }, - { verboseLoggingEnabled: true } + { runLoggingEnabled: true } ); selectRawAppointmentIds = createParameterizedRootSelector( function _selectRawAppointmentIds(state) { @@ -99,7 +99,7 @@ describe('Overlapping dependencies', () => { dateObject: makeDateObjectForDay(rawAppointmentData.dayNum), }; }, - { verboseLoggingEnabled: true } + { runLoggingEnabled: true } ); selectAllAppointments = createParameterizedSelector( From 83b919b9a4c6a00fc238f1d0b60bca2aedabb79f Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Mon, 10 Dec 2018 15:26:28 -0500 Subject: [PATCH 3/8] Some dependency-record and result-record fixes --- src/defaultOptions.js | 2 +- src/parameterizedSelectorFactory.js | 150 +++++++++++++++------------- tests/overlapping-dependencies.js | 2 +- 3 files changed, 83 insertions(+), 71 deletions(-) diff --git a/src/defaultOptions.js b/src/defaultOptions.js index 0e9f189..e919565 100644 --- a/src/defaultOptions.js +++ b/src/defaultOptions.js @@ -16,7 +16,7 @@ const willThrowErrorIfNotSet = label => () => { * if the consumer ever needs to reference the original value, after setting new defaults. */ const defaultInitialOptions = { - displayNamePrefix: 'parameterizedSelector', + displayNamePrefix: 'parameterizedSelector:', compareIncomingStates: COMPARISON_PRESETS.SAME_REFERENCE, compareSelectorResults: COMPARISON_PRESETS.SHALLOW_EQUAL, exceptionCallback: (errorMessage, error) => { diff --git a/src/parameterizedSelectorFactory.js b/src/parameterizedSelectorFactory.js index 55a8fe8..3f9cd0f 100644 --- a/src/parameterizedSelectorFactory.js +++ b/src/parameterizedSelectorFactory.js @@ -14,6 +14,10 @@ const getTopCallStackEntry = () => parameterizedSelectorCallStack.length && parameterizedSelectorCallStack[parameterizedSelectorCallStack.length - 1]; /** + * As selectors are executed, we use call stack entries to know which 'child' selectors each 'parent' + * depends on. We track both the immediate dependencies and (for performance) the eventual root selectors + * that get called, for each set of params. + * * For performance, this function ensures all entries on the call stack have the same shape. * `state` and `hasStaticDependencies` are mandatory for each call. */ @@ -22,7 +26,6 @@ const pushCallStackEntry = (state, hasStaticDependencies, overrideValues = {}) = const callStackEntry = { state, - hasStaticDependencies, rootDependencies: [], ownDependencies: [], canReRun: topOfCallStack ? topOfCallStack.canReRun : true, @@ -39,6 +42,13 @@ const popCallStackEntry = () => parameterizedSelectorCallStack.pop(); /** + * A "result record" tracks the last result -- anywhere/globally -- of running the selector with a particular + * set of params. These records are mutated. + * + * These are used with "dependency records" (which are parent-specific instead of global) to determine + * whether a selector needs to re-run: if either the result record is out-of-date or the dependency record + * doesn't match it, then the dependency needs to be re-checked. + * * Like pushCallStackEntry, this function ensures that the resultRecord objects always have the same shape, * and requires `state` be passed as a required arg. */ @@ -50,7 +60,7 @@ const createResultRecord = (state, previousResult = {}, overrideValues = {}) => hasReturnValue: false, returnValue: null, error: null, - // Note that these counts must be incremented separately (if performance checks are on) + // This record gets created only after the invoke starts, so it begins at 1 invokeCount: previousResult.invokeCount || 1, skippedRunCount: previousResult.skippedRunCount || 0, phantomRunCount: previousResult.phantomRunCount || 0, @@ -62,19 +72,19 @@ const createResultRecord = (state, previousResult = {}, overrideValues = {}) => }; -const hasAnyDependencyChanged = (state, dependencyList, options, loggingPrefix, additionalArgs = []) => { - const dependencyListLength = dependencyList.length; +const hasAnyDependencyChanged = (state, dependencyRecordList, options, loggingPrefix, additionalArgs = []) => { + const dependencyListLength = dependencyRecordList.length; for (let i = 0; i < dependencyListLength; i += 1) { - const [dependencySelector, dependencyKeyParams, dependencyReturnValue] = dependencyList[i]; + const [dependencySelector, dependencyKeyParams, dependencyReturnValue] = dependencyRecordList[i]; // Does our dependency have anything new? - const result = dependencySelector.directRunFromParent(state, dependencyKeyParams, ...additionalArgs); + const resultRecord = dependencySelector.directRunFromParent(state, dependencyKeyParams, ...additionalArgs); // The selector function itself returns some additional metadata alongside the returnValue, // to cover exceptions and edge cases like not being able to run. const { hasReturnValue, returnValue: newReturnValue, - } = result; + } = resultRecord; if (!hasReturnValue) { if (options.verboseLoggingEnabled) { @@ -120,10 +130,10 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { if (!options.displayName) { const functionDisplayName = innerFn.displayName || innerFn.name; if (functionDisplayName) { - options.displayName = `${options.displayNamePrefix}(${functionDisplayName})`; + options.displayName = `${options.displayNamePrefix}${functionDisplayName}`; } else { numUnnamedSelectors += 1; - options.displayName = `${options.displayNamePrefix}(#${numUnnamedSelectors})`; + options.displayName = `${options.displayNamePrefix}${numUnnamedSelectors}`; } } if (options.verboseLoggingEnabled) { @@ -148,11 +158,11 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { * where each resultRecord looks like: { * state, * rootDependencies: [ - * [parameterizedSelector, keyParams, returnValue], + * [parameterizedSelector, keyParams, returnValue], // dependency records * ... * ], * ownDependencies: [ - * [parameterizedSelector, keyParams, returnValue], + * [parameterizedSelector, keyParams, returnValue], // dependency records * ... * ], * hasReturnValue, @@ -165,7 +175,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { * abortedRunCount, * } */ - const previousResultsByParam = {}; + const resultRecordsByParam = {}; /** * Here we can track the number of recomputations due to cache misses, state changes, param changes etc, @@ -218,9 +228,9 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { const parentCaller = getTopCallStackEntry(); const keyParamsString = createKeyFromParams(keyParams); - const previousResult = previousResultsByParam[keyParamsString]; + const previousResultRecord = resultRecordsByParam[keyParamsString]; - const loggingPrefix = `Parameterized selector "${options.displayName}(${keyParamsString})"`; + const loggingPrefix = `"${options.displayName}(${keyParamsString})"`; if (options.verboseLoggingEnabled && options.useConsoleGroup) { console.groupCollapsed(`Starting ${loggingPrefix}`, { // eslint-disable-line no-console @@ -229,7 +239,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { keyParams, keyParamsString, additionalArgs, - previousResult, + previousResultRecord, }); } else if (options.verboseLoggingEnabled) { options.verboseLoggingCallback(`Starting ${loggingPrefix}`, { @@ -238,7 +248,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { keyParams, keyParamsString, additionalArgs, - previousResult, + previousResultRecord, }); } @@ -260,8 +270,8 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { if (options.performanceChecksEnabled) { globalInvokeCount += 1; - if (previousResult) { - previousResult.invokeCount += 1; + if (previousResultRecord) { + previousResultRecord.invokeCount += 1; } } if (options.runLoggingEnabled) { @@ -274,14 +284,14 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { // Step 1: Do we have a prior result for this parameterizedSelector + its keyParams? let canUsePreviousResult = false; // until proven otherwise - if (previousResult && previousResult.hasReturnValue) { + if (previousResultRecord && previousResultRecord.hasReturnValue) { const { state: previousState, rootDependencies: previousRootDependencies, ownDependencies: previousOwnDependencies, // Note that invokeCount, skippedRunCount, phantomRunCount, fullRunCount, and abortedRunCount // are only referenced if they're actually in use. - } = previousResult; + } = previousResultRecord; // Step 2: Have we already run with these params for this state? // compareIncomingStates is only honored for root selectors @@ -294,10 +304,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { if (options.verboseLoggingEnabled) { options.verboseLoggingCallback(`${loggingPrefix} is cached: state hasn't changed`); } - } else if (!isRootSelector && ( - previousRootDependencies.length > 0 - || previousOwnDependencies.length > 0 - )) { + } else if (!isRootSelector && (previousRootDependencies.length > 0 || previousOwnDependencies.length > 0)) { // Step 3: Have any of our dependencies changed? // @TODO: Need to warn if a root selector ever has dependencies @@ -307,16 +314,18 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { // Since we're only checking dependencies, we want to minimize any extra work the child selectors // could do. - pushCallStackEntry(state, hasStaticDependencies, { - shouldRecordDependencies: false, - }); + // pushCallStackEntry(state, hasStaticDependencies, { + // @BUG? + // shouldRecordDependencies: false, + // canReRun: false, + // }); // We'll check the root dependencies first: if one of them has changed, we'll check out intermediates. // If one of those has also changed, then we need to rerun. const hasChanges = hasAnyDependencyChanged(state, previousRootDependencies, options, loggingPrefix, additionalArgs) // eslint-disable-line max-len && hasAnyDependencyChanged(state, previousOwnDependencies, options, loggingPrefix, additionalArgs); - popCallStackEntry(); + // popCallStackEntry(); if (!hasChanges) { canUsePreviousResult = true; @@ -332,7 +341,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { let newResult; if (canUsePreviousResult) { - newResult = previousResult; + newResult = previousResultRecord; if (options.performanceChecksEnabled) { globalSkippedRunCount += 1; @@ -347,7 +356,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { } } else { // Step 4: Run and obtain a new result, if we can. - newResult = createResultRecord(state, previousResult); + newResult = createResultRecord(state, previousResultRecord); if (parentCaller.canReRun) { // Collect dependencies, if appropriate @@ -384,16 +393,16 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { const callStackEntry = popCallStackEntry(); // Step 5: Did we really get back a new value? - if (previousResult && previousResult.hasReturnValue && newResult.hasReturnValue - && compareSelectorResults(previousResult.returnValue, newResult.returnValue) + if (previousResultRecord && previousResultRecord.hasReturnValue && newResult.hasReturnValue + && compareSelectorResults(previousResultRecord.returnValue, newResult.returnValue) ) { // We got back the same result: return what we had before (but update its state so we don't need // to check it again) - newResult = previousResult; + newResult = previousResultRecord; newResult.state = state; if (options.verboseLoggingEnabled) { options.verboseLoggingCallback(`${loggingPrefix} didn't need to re-run: the result is the same`, { - previousResult, + previousResultRecord, newResult, }); } @@ -411,7 +420,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { } } else { // It really IS new! - previousResultsByParam[keyParamsString] = newResult; + resultRecordsByParam[keyParamsString] = newResult; if (options.verboseLoggingEnabled) { options.verboseLoggingCallback(`${loggingPrefix} has a new return value: `, newResult.returnValue); } @@ -458,7 +467,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { } else { // We need to re-run, but the parentCaller told us not to, so the default `hasReturnValue: false` // will pass through. - previousResultsByParam[keyParamsString] = newResult; + resultRecordsByParam[keyParamsString] = newResult; if (options.performanceChecksEnabled) { globalAbortedRunCount += 1; @@ -478,21 +487,26 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { // know that this one was called, regardless of our cached/dirty state. if (parentCaller && parentCaller.shouldRecordDependencies) { // @TODO: Split this into separate functions so that they can be ordered in definition order - // eslint-disable-next-line no-use-before-define - const thisResultRecord = [parameterizedSelector, keyParams, newResult.returnValue]; // Regardless of whether or not it's a root dependency, we need to track it as *our own* immediate dependency - parentCaller.ownDependencies.push(thisResultRecord); + parentCaller.ownDependencies.push( + // eslint-disable-next-line no-use-before-define + [parameterizedSelector, keyParams, newResult.returnValue], + ); if (isRootSelector) { const callStackLength = parameterizedSelectorCallStack.length; for (let i = 0; i < callStackLength; i += 1) { - parameterizedSelectorCallStack[i].rootDependencies.push(thisResultRecord); + parameterizedSelectorCallStack[i].rootDependencies.push( + // *Each* items in the stack gets its own, separate copy of the dependencyRecord + // eslint-disable-next-line no-use-before-define + [parameterizedSelector, keyParams, newResult.returnValue], + ); } } } if (options.verboseLoggingEnabled) { - if (newResult === previousResult) { + if (newResult === previousResultRecord) { options.verboseLoggingCallback(`${loggingPrefix} is done, with no change`); } else { options.verboseLoggingCallback(`${loggingPrefix} is done, with a new result: `, newResult); @@ -562,22 +576,20 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { * decide when/whether to re-run the dependencies to see if they've changed. */ parameterizedSelector.hasCachedResult = (...args) => { - const topOfCallStack = !getTopCallStackEntry(); + const parentCaller = getTopCallStackEntry(); const argsWithState = getArgumentsFromExternalCall(args); - if (topOfCallStack) { - pushCallStackEntry(argsWithState[0], hasStaticDependencies, { - rootDependencies: topOfCallStack.rootDependencies, - ownDependencies: topOfCallStack.ownDependencies, - canReRun: false, - }); - } else { + // if (parentCaller) { + // pushCallStackEntry(argsWithState[0], hasStaticDependencies, { + // // When checking for results + // ownDependencies: parentCaller.ownDependencies, + // canReRun: false, + // }); + // } else { pushCallStackEntry(argsWithState[0], hasStaticDependencies, { - rootDependencies: [], - ownDependencies: [], canReRun: false, }); - } + // } const result = evaluateParameterizedSelector(...argsWithState); popCallStackEntry(); @@ -599,39 +611,39 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { parameterizedSelector.getInvokeCountForParams = (keyParams) => { const keyParamsString = createKeyFromParams(keyParams); - const previousResult = previousResultsByParam[keyParamsString]; - return previousResult ? previousResult.invokeCount : 0; + const resultRecord = resultRecordsByParam[keyParamsString]; + return resultRecord ? resultRecord.invokeCount : 0; }; parameterizedSelector.getSkippedRunCountForParams = (keyParams) => { const keyParamsString = createKeyFromParams(keyParams); - const previousResult = previousResultsByParam[keyParamsString]; - return previousResult ? previousResult.skippedRunCount : 0; + const resultRecord = resultRecordsByParam[keyParamsString]; + return resultRecord ? resultRecord.skippedRunCount : 0; }; parameterizedSelector.getPhantomRunCountForParams = (keyParams) => { const keyParamsString = createKeyFromParams(keyParams); - const previousResult = previousResultsByParam[keyParamsString]; - return previousResult ? previousResult.phantomRunCount : 0; + const resultRecord = resultRecordsByParam[keyParamsString]; + return resultRecord ? resultRecord.phantomRunCount : 0; }; parameterizedSelector.getFullRunCountForParams = (keyParams) => { const keyParamsString = createKeyFromParams(keyParams); - const previousResult = previousResultsByParam[keyParamsString]; - return previousResult ? previousResult.fullRunCount : 0; + const resultRecord = resultRecordsByParam[keyParamsString]; + return resultRecord ? resultRecord.fullRunCount : 0; }; parameterizedSelector.getAbortedRunCountForParams = (keyParams) => { const keyParamsString = createKeyFromParams(keyParams); - const previousResult = previousResultsByParam[keyParamsString]; - return previousResult ? previousResult.abortedRunCount : 0; + const resultRecord = resultRecordsByParam[keyParamsString]; + return resultRecord ? resultRecord.abortedRunCount : 0; }; parameterizedSelector.getAllCountsForParams = (keyParams) => { const keyParamsString = createKeyFromParams(keyParams); - const previousResult = previousResultsByParam[keyParamsString]; - return previousResult + const resultRecord = resultRecordsByParam[keyParamsString]; + return resultRecord ? { - invokeCount: previousResult.invokeCount, - skippedRunCount: previousResult.skippedRunCount, - phantomRunCount: previousResult.phantomRunCount, - fullRunCount: previousResult.fullRunCount, - abortedRunCount: previousResult.abortedRunCount, + invokeCount: resultRecord.invokeCount, + skippedRunCount: resultRecord.skippedRunCount, + phantomRunCount: resultRecord.phantomRunCount, + fullRunCount: resultRecord.fullRunCount, + abortedRunCount: resultRecord.abortedRunCount, } : { invokeCount: 0, skippedRunCount: 0, diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js index 991a9cd..ccad658 100644 --- a/tests/overlapping-dependencies.js +++ b/tests/overlapping-dependencies.js @@ -147,7 +147,7 @@ describe('Overlapping dependencies', () => { assertCountsForParams(selectRawAppointmentData, 1, { invokeCount: 2, fullRunCount: 1, - skippedRunCount: 1, + phantomRunCount: 1, }); assertCountsForParams(selectAppointmentById, { appointmentId: 1 }, { invokeCount: 2, From 526c1a3fec028ea88c4022bd497eae704ea54aa4 Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Mon, 10 Dec 2018 18:40:08 -0500 Subject: [PATCH 4/8] Bugfixes and several debug improvements --- README.md | 4 +- src/defaultOptions.js | 1 + src/parameterizedSelectorFactory.js | 355 +++++++++++++++------------- tests/functional-scenarios.test.js | 4 +- tests/overlapping-dependencies.js | 38 ++- 5 files changed, 222 insertions(+), 180 deletions(-) diff --git a/README.md b/README.md index 5eaae93..296dbe0 100644 --- a/README.md +++ b/README.md @@ -172,7 +172,8 @@ Settable at any time: Name | Type | Description --- | --- | --- displayName | String | A human-readable name for the function, used for debug output and warnings. -useConsoleGroup | Boolean | Will cause verbose logging to be nested in console groups. +useConsoleGroup | Boolean | Will wrap other logging calls in console groups. +runLoggingEnabled | Boolean | Will output general log information at the beginning and end of each selector invokation. verboseLoggingEnabled | Boolean | Will fill your console with far too much debug info. This will be cleaned up in the near future. verboseLoggingCallback | Function | Gets called for every verboseLogging item; this is `console.log` by default. performanceChecksEnabled | Boolean | Will give you warnings or pings if something causes a selector to re-run unnecessarily, or if it encounters other bad smells. (Not yet implemented.) @@ -185,3 +186,4 @@ onSkippedRun | Function | Callback fired when a selector returns its cached valu onPhantomRun | Function | Callback fired when a selector runs but returns something equivalent to its cached value. Useful for debugging. onFullRun | Function | Callback fired when a selector runs and returns a new value. Useful for debugging. onAbortedRun | Function | Callback fired when a selector needs to run but isn't allowed to because it's being queried (e.g., for `hasCachedResult`.) Useful for debugging. +onErrorRun | Function | Callback fired when a selector throws an exception. Useful for debugging. diff --git a/src/defaultOptions.js b/src/defaultOptions.js index e919565..d48bb8e 100644 --- a/src/defaultOptions.js +++ b/src/defaultOptions.js @@ -53,6 +53,7 @@ const defaultOptions = { onPhantomRun: null, onFullRun: null, onAbortedRun: null, + onErrorRun: null, }; // Note that there is no `setDefaultOptions`: diff --git a/src/parameterizedSelectorFactory.js b/src/parameterizedSelectorFactory.js index 3f9cd0f..88f8077 100644 --- a/src/parameterizedSelectorFactory.js +++ b/src/parameterizedSelectorFactory.js @@ -43,7 +43,7 @@ const popCallStackEntry = () => parameterizedSelectorCallStack.pop(); /** * A "result record" tracks the last result -- anywhere/globally -- of running the selector with a particular - * set of params. These records are mutated. + * state and set of params. These records are mutated and persistent. * * These are used with "dependency records" (which are parent-specific instead of global) to determine * whether a selector needs to re-run: if either the result record is out-of-date or the dependency record @@ -52,24 +52,20 @@ const popCallStackEntry = () => parameterizedSelectorCallStack.pop(); * Like pushCallStackEntry, this function ensures that the resultRecord objects always have the same shape, * and requires `state` be passed as a required arg. */ -const createResultRecord = (state, previousResult = {}, overrideValues = {}) => { - const result = { - state, - rootDependencies: previousResult.rootDependencies || [], - ownDependencies: previousResult.ownDependencies || [], - hasReturnValue: false, - returnValue: null, - error: null, - // This record gets created only after the invoke starts, so it begins at 1 - invokeCount: previousResult.invokeCount || 1, - skippedRunCount: previousResult.skippedRunCount || 0, - phantomRunCount: previousResult.phantomRunCount || 0, - fullRunCount: previousResult.fullRunCount || 0, - abortedRunCount: previousResult.abortedRunCount || 0, - ...overrideValues, - }; - return result; -}; +const createResultRecord = state => ({ + state, + rootDependencies: [], + ownDependencies: [], + hasReturnValue: false, + returnValue: null, + error: null, + invokeCount: 0, + skippedRunCount: 0, + phantomRunCount: 0, + fullRunCount: 0, + abortedRunCount: 0, + errorRunCount: 0, +}); const hasAnyDependencyChanged = (state, dependencyRecordList, options, loggingPrefix, additionalArgs = []) => { @@ -173,6 +169,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { * phantomRunCount, * fullRunCount, * abortedRunCount, + * errorRunCount, * } */ const resultRecordsByParam = {}; @@ -183,13 +180,14 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { * This is primarily used for performance and unit-testing purposes. * * Note that these counts apply across ALL params for the selector. There is a separate set of per-param - * counters, tracked in the previousResults. + * counters, tracked in the individual resultRecords. */ let globalInvokeCount = 0; let globalSkippedRunCount = 0; let globalPhantomRunCount = 0; let globalFullRunCount = 0; let globalAbortedRunCount = 0; + let globalErrorRunCount = 0; /** @@ -228,7 +226,11 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { const parentCaller = getTopCallStackEntry(); const keyParamsString = createKeyFromParams(keyParams); - const previousResultRecord = resultRecordsByParam[keyParamsString]; + + if (!hasOwnProperty.call(resultRecordsByParam, keyParamsString)) { + resultRecordsByParam[keyParamsString] = createResultRecord(state); + } + const resultRecord = resultRecordsByParam[keyParamsString]; const loggingPrefix = `"${options.displayName}(${keyParamsString})"`; @@ -239,7 +241,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { keyParams, keyParamsString, additionalArgs, - previousResultRecord, + resultRecord, }); } else if (options.verboseLoggingEnabled) { options.verboseLoggingCallback(`Starting ${loggingPrefix}`, { @@ -248,7 +250,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { keyParams, keyParamsString, additionalArgs, - previousResultRecord, + resultRecord, }); } @@ -270,28 +272,25 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { if (options.performanceChecksEnabled) { globalInvokeCount += 1; - if (previousResultRecord) { - previousResultRecord.invokeCount += 1; - } + resultRecord.invokeCount += 1; } if (options.runLoggingEnabled) { console.group(`Invoke: ${loggingPrefix}`); } if (typeof options.onInvoke === 'function') { - options.onInvoke(/* @TODO: What should go here? */); + options.onInvoke(keyParams, resultRecord); } // Step 1: Do we have a prior result for this parameterizedSelector + its keyParams? let canUsePreviousResult = false; // until proven otherwise - if (previousResultRecord && previousResultRecord.hasReturnValue) { + if (resultRecord.hasReturnValue) { const { state: previousState, rootDependencies: previousRootDependencies, ownDependencies: previousOwnDependencies, - // Note that invokeCount, skippedRunCount, phantomRunCount, fullRunCount, and abortedRunCount - // are only referenced if they're actually in use. - } = previousResultRecord; + // Note that the count vars are only referenced if they're actually in use. + } = resultRecord; // Step 2: Have we already run with these params for this state? // compareIncomingStates is only honored for root selectors @@ -312,21 +311,11 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { options.verboseLoggingCallback(`${loggingPrefix} is checking its dependencies for changes...`); } - // Since we're only checking dependencies, we want to minimize any extra work the child selectors - // could do. - // pushCallStackEntry(state, hasStaticDependencies, { - // @BUG? - // shouldRecordDependencies: false, - // canReRun: false, - // }); - - // We'll check the root dependencies first: if one of them has changed, we'll check out intermediates. + // We'll check the root dependencies first: if one of them has changed, we'll check our own direct dependencies. // If one of those has also changed, then we need to rerun. const hasChanges = hasAnyDependencyChanged(state, previousRootDependencies, options, loggingPrefix, additionalArgs) // eslint-disable-line max-len && hasAnyDependencyChanged(state, previousOwnDependencies, options, loggingPrefix, additionalArgs); - // popCallStackEntry(); - if (!hasChanges) { canUsePreviousResult = true; if (options.verboseLoggingEnabled) { @@ -336,149 +325,156 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { } } - // We need to return a bunch of metaData along with the returnValue, at the very end. Instead of tracking - // a handful of separate variables, everything will be accumulated here. - let newResult; + // Regardless of whether we skip, rerun, phantom run, etc, we need to note that we did this work for + // this particular state. + resultRecord.state = state; if (canUsePreviousResult) { - newResult = previousResultRecord; - if (options.performanceChecksEnabled) { globalSkippedRunCount += 1; - newResult.skippedRunCount += 1; + resultRecord.skippedRunCount += 1; } if (options.runLoggingEnabled) { options.runLoggingCallback(`Skipped run: ${loggingPrefix}`); console.groupEnd(); } + if (typeof options.onSkippedRun === 'function') { - options.onSkippedRun(/* @TODO: What should go here? */); + options.onSkippedRun(keyParams, resultRecord); + } + } else if (!parentCaller.canReRun) { + // We need to re-run, but the parentCaller told us not to + resultRecord.hasReturnValue = false; + + if (options.performanceChecksEnabled) { + globalAbortedRunCount += 1; + resultRecord.abortedRunCount += 1; + } + if (options.runLoggingEnabled) { + options.runLoggingCallback(`Aborted run: ${loggingPrefix}`); + console.groupEnd(); + } + if (typeof options.onAbortedRun === 'function') { + options.onAbortedRun(keyParams, resultRecord); } } else { // Step 4: Run and obtain a new result, if we can. - newResult = createResultRecord(state, previousResultRecord); - - if (parentCaller.canReRun) { - // Collect dependencies, if appropriate - pushCallStackEntry(state, hasStaticDependencies); - - try { - let returnValue; - if (isRootSelector) { - if (options.verboseLoggingEnabled) { - options.verboseLoggingCallback(`Running ${loggingPrefix} as a root selector`, { - state, keyParams, additionalArgs, - }); - } - returnValue = innerFn(state, keyParams, ...additionalArgs); - } else { - if (options.verboseLoggingEnabled) { - options.verboseLoggingCallback(`Running ${loggingPrefix} as a normal selector`, { - state, keyParams, additionalArgs, - }); - } - returnValue = innerFn(keyParams, ...additionalArgs); - } - // If we reach this point without error, all is well - newResult.hasReturnValue = true; - newResult.returnValue = returnValue; - } catch (errorFromInnerFn) { - newResult.error = errorFromInnerFn; - - options.warningsCallback(`${loggingPrefix} threw an exception: ${newResult.error.message}`, newResult.error); - if (options.warningsEnabled) { - console.trace(); // eslint-disable-line no-console - } - } - const callStackEntry = popCallStackEntry(); - - // Step 5: Did we really get back a new value? - if (previousResultRecord && previousResultRecord.hasReturnValue && newResult.hasReturnValue - && compareSelectorResults(previousResultRecord.returnValue, newResult.returnValue) - ) { - // We got back the same result: return what we had before (but update its state so we don't need - // to check it again) - newResult = previousResultRecord; - newResult.state = state; + const oldReturnValue = resultRecord.returnValue; + let newReturnValue; + let newError; + + pushCallStackEntry(state, hasStaticDependencies); + try { + if (isRootSelector) { if (options.verboseLoggingEnabled) { - options.verboseLoggingCallback(`${loggingPrefix} didn't need to re-run: the result is the same`, { - previousResultRecord, - newResult, + options.verboseLoggingCallback(`Running ${loggingPrefix} as a root selector`, { + state, keyParams, additionalArgs, }); } - - if (options.performanceChecksEnabled) { - globalPhantomRunCount += 1; - newResult.phantomRunCount += 1; - } - if (options.runLoggingEnabled) { - options.runLoggingCallback(`Phantom run: ${loggingPrefix}`); - console.groupEnd(); - } - if (typeof options.onPhantomRun === 'function') { - options.onPhantomRun(/* @TODO: What should go here? */); - } + newReturnValue = innerFn(state, keyParams, ...additionalArgs); } else { - // It really IS new! - resultRecordsByParam[keyParamsString] = newResult; if (options.verboseLoggingEnabled) { - options.verboseLoggingCallback(`${loggingPrefix} has a new return value: `, newResult.returnValue); + options.verboseLoggingCallback(`Running ${loggingPrefix} as a normal selector`, { + state, keyParams, additionalArgs, + }); } + newReturnValue = innerFn(keyParams, ...additionalArgs); + } + // If we reach this point without error, all is well + } catch (errorFromInnerFn) { + newError = errorFromInnerFn; + } + const callStackEntry = popCallStackEntry(); - if (options.performanceChecksEnabled) { - globalFullRunCount += 1; - newResult.fullRunCount += 1; - } - if (options.runLoggingEnabled) { - options.runLoggingCallback(`Full run: ${loggingPrefix}`); - console.groupEnd(); - } - if (typeof options.onFullRun === 'function') { - options.onFullRun(/* @TODO: What should go here? */); - } + // Step 5: Let's look at what we got back + if (newError) { + // Oh no! + resultRecord.returnValue = undefined; + resultRecord.hasReturnValue = false; + resultRecord.error = newError; + + if (options.performanceChecksEnabled) { + globalErrorRunCount += 1; + resultRecord.errorRunCount += 1; } - if (!newResult.error && ( - callStackEntry.rootDependencies.length - || callStackEntry.ownDependencies.length - )) { - // Carry over the bookkeeping records of whatever sub-selectors were run within innerFn. - newResult.rootDependencies = callStackEntry.rootDependencies; - newResult.ownDependencies = callStackEntry.ownDependencies; - - if (options.warningsEnabled && isRootSelector) { - options.warningsCallback(`${loggingPrefix} is supposed to be a root selector, but it recorded dependencies`, { - callStackEntry, - }); - // @TODO: Maybe add some intermittent checks around hasStaticDependencies when in dev mode, - // to have it warn if something should be static but isn't, or if it always records the same set. - } + if (options.warningsEnabled) { + options.warningsCallback(`${loggingPrefix} threw an exception: ${newError.message}`, newError); + console.trace(); // eslint-disable-line no-console + } + if (options.runLoggingEnabled) { + options.runLoggingCallback(`Error run: ${loggingPrefix}`); + console.groupEnd(); + } + if (typeof options.onErrorRun === 'function') { + options.onErrorRun(oldReturnValue, undefined, keyParams, resultRecord); + } + } else if (resultRecord.hasReturnValue && compareSelectorResults(oldReturnValue, newReturnValue)) { + // We got back an equivalent result to what we had before. No need to update. + if (options.verboseLoggingEnabled) { + options.verboseLoggingCallback(`${loggingPrefix} didn't need to re-run: the result is the same`, { + resultRecord, + }); } if (options.performanceChecksEnabled) { - // While we're here, let's make sure the selector isn't recomputing too often. - // @TODO: Make overrideable options for these values - if (newResult.invokeCount > 5 && newResult.fullRunCount > 0.75 * newResult.invokeCount) { - options.performanceChecksCallback(`${loggingPrefix} is recomputing a lot: ${newResult.fullRunCount} of ${newResult.invokeCount} runs gave new results.`); - } else if (globalInvokeCount > 25 && globalFullRunCount > 0.75 * globalInvokeCount) { - options.performanceChecksCallback(`${options.displayName} is recomputing a lot in total: ${globalFullRunCount} of ${globalInvokeCount} runs gave new results.`); - } + globalPhantomRunCount += 1; + resultRecord.phantomRunCount += 1; + } + if (options.runLoggingEnabled) { + options.runLoggingCallback(`Phantom run: ${loggingPrefix}`); + console.groupEnd(); + } + if (typeof options.onPhantomRun === 'function') { + // To better align with the other after-run callbacks, the 'new' value is sent as if it were the old value, + // so that the actual return value comes second. + options.onPhantomRun(newReturnValue, oldReturnValue, keyParams, resultRecord); } } else { - // We need to re-run, but the parentCaller told us not to, so the default `hasReturnValue: false` - // will pass through. - resultRecordsByParam[keyParamsString] = newResult; + // It's a genuinely new value! + resultRecord.returnValue = newReturnValue; + resultRecord.hasReturnValue = true; + resultRecord.error = undefined; + + if (options.verboseLoggingEnabled) { + options.verboseLoggingCallback(`${loggingPrefix} has a new return value: `, newReturnValue); + } if (options.performanceChecksEnabled) { - globalAbortedRunCount += 1; - newResult.abortedRunCount += 1; + globalFullRunCount += 1; + resultRecord.fullRunCount += 1; } if (options.runLoggingEnabled) { - options.runLoggingCallback(`Aborted run: ${loggingPrefix}`); + options.runLoggingCallback(`Full run: ${loggingPrefix}`); console.groupEnd(); } - if (typeof options.onAbortedRun === 'function') { - options.onAbortedRun(/* @TODO: What should go here? */); + if (typeof options.onFullRun === 'function') { + options.onFullRun(oldReturnValue, newReturnValue, keyParams, resultRecord); + } + } + // At this point, the values in resultRecord are all up-to-date + + if (!resultRecord.error && (callStackEntry.rootDependencies.length || callStackEntry.ownDependencies.length)) { + // Update our dependencyRecords, so we know which sub-selectors were run within innerFn. + resultRecord.rootDependencies = callStackEntry.rootDependencies; + resultRecord.ownDependencies = callStackEntry.ownDependencies; + + if (options.warningsEnabled && isRootSelector) { + options.warningsCallback(`${loggingPrefix} is supposed to be a root selector, but it recorded dependencies`, { + callStackEntry, + }); + // @TODO: Maybe add some intermittent checks around hasStaticDependencies when in dev mode, + // to have it warn if something should be static but isn't, or if it always records the same set. + } + } + + if (options.performanceChecksEnabled) { + // While we're here, let's make sure the selector isn't recomputing too often. + // @TODO: Make overrideable options for these values + if (resultRecord.invokeCount > 5 && resultRecord.fullRunCount > 0.75 * resultRecord.invokeCount) { + options.performanceChecksCallback(`${loggingPrefix} is recomputing a lot: ${resultRecord.fullRunCount} of ${resultRecord.invokeCount} runs gave new results.`); + } else if (globalInvokeCount > 25 && globalFullRunCount > 0.75 * globalInvokeCount) { + options.performanceChecksCallback(`${options.displayName} is recomputing a lot in total: ${globalFullRunCount} of ${globalInvokeCount} runs gave new results.`); } } } @@ -490,33 +486,32 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { // Regardless of whether or not it's a root dependency, we need to track it as *our own* immediate dependency parentCaller.ownDependencies.push( // eslint-disable-next-line no-use-before-define - [parameterizedSelector, keyParams, newResult.returnValue], + [parameterizedSelector, keyParams, resultRecord.returnValue], ); + } - if (isRootSelector) { - const callStackLength = parameterizedSelectorCallStack.length; - for (let i = 0; i < callStackLength; i += 1) { - parameterizedSelectorCallStack[i].rootDependencies.push( + if (isRootSelector) { + const callStackLength = parameterizedSelectorCallStack.length; + for (let i = 0; i < callStackLength; i += 1) { + const callStackEntry = parameterizedSelectorCallStack[i]; + if (callStackEntry.shouldRecordDependencies) { + callStackEntry.rootDependencies.push( // *Each* items in the stack gets its own, separate copy of the dependencyRecord // eslint-disable-next-line no-use-before-define - [parameterizedSelector, keyParams, newResult.returnValue], + [parameterizedSelector, keyParams, resultRecord.returnValue], ); } } } if (options.verboseLoggingEnabled) { - if (newResult === previousResultRecord) { - options.verboseLoggingCallback(`${loggingPrefix} is done, with no change`); - } else { - options.verboseLoggingCallback(`${loggingPrefix} is done, with a new result: `, newResult); - } + options.verboseLoggingCallback(`${loggingPrefix} is done`, resultRecord); if (options.useConsoleGroup) { console.groupEnd(); // eslint-disable-line no-console } } - return newResult; + return resultRecord; }; @@ -564,10 +559,20 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { return result.returnValue; } - // This lets selectors bypass the wrappers internally, when appropriate. It shouldn't be called from - // outside of this file (and tests), though. + /** + * This lets selectors bypass the wrappers internally, when appropriate. It shouldn't be called from + * outside of this file (and tests), though. + */ parameterizedSelector.directRunFromParent = evaluateParameterizedSelector; + /** + * This is useful for debugging, but should not be used otherwise. + */ + parameterizedSelector.getResultRecord = (keyParams) => { + const keyParamsString = createKeyFromParams(keyParams); + return resultRecordsByParam[keyParamsString]; + }; + /** * This offers a way to inspect a parameterizedSelector call's status without calling it. * Note that *dependencies* may be called, however, to determine whether their values are still valid. @@ -576,7 +581,7 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { * decide when/whether to re-run the dependencies to see if they've changed. */ parameterizedSelector.hasCachedResult = (...args) => { - const parentCaller = getTopCallStackEntry(); + // const parentCaller = getTopCallStackEntry(); const argsWithState = getArgumentsFromExternalCall(args); // if (parentCaller) { @@ -586,9 +591,9 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { // canReRun: false, // }); // } else { - pushCallStackEntry(argsWithState[0], hasStaticDependencies, { - canReRun: false, - }); + pushCallStackEntry(argsWithState[0], hasStaticDependencies, { + canReRun: false, + }); // } const result = evaluateParameterizedSelector(...argsWithState); popCallStackEntry(); @@ -601,12 +606,14 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { parameterizedSelector.getGlobalPhantomRunCount = () => globalPhantomRunCount; parameterizedSelector.getGlobalFullRunCount = () => globalFullRunCount; parameterizedSelector.getGlobalAbortedRunCount = () => globalAbortedRunCount; + parameterizedSelector.getGlobalErrorRunCount = () => globalErrorRunCount; parameterizedSelector.getAllGlobalCounts = () => ({ globalInvokeCount, globalSkippedRunCount, globalPhantomRunCount, globalFullRunCount, globalAbortedRunCount, + globalErrorRunCount, }); parameterizedSelector.getInvokeCountForParams = (keyParams) => { @@ -634,6 +641,12 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { const resultRecord = resultRecordsByParam[keyParamsString]; return resultRecord ? resultRecord.abortedRunCount : 0; }; + parameterizedSelector.getErrorRunCountForParams = (keyParams) => { + const keyParamsString = createKeyFromParams(keyParams); + const resultRecord = resultRecordsByParam[keyParamsString]; + return resultRecord ? resultRecord.errorRunCount : 0; + }; + parameterizedSelector.getAllCountsForParams = (keyParams) => { const keyParamsString = createKeyFromParams(keyParams); const resultRecord = resultRecordsByParam[keyParamsString]; @@ -644,12 +657,14 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { phantomRunCount: resultRecord.phantomRunCount, fullRunCount: resultRecord.fullRunCount, abortedRunCount: resultRecord.abortedRunCount, + errorRunCount: resultRecord.errorRunCount, } : { invokeCount: 0, skippedRunCount: 0, phantomRunCount: 0, fullRunCount: 0, abortedRunCount: 0, + errorRunCount: 0, }; }; diff --git a/tests/functional-scenarios.test.js b/tests/functional-scenarios.test.js index 52ac942..31265b4 100644 --- a/tests/functional-scenarios.test.js +++ b/tests/functional-scenarios.test.js @@ -228,10 +228,10 @@ describe('Selectors for single objects', () => { assert.equal(selectAuthor.getInvokeCountForParams({ authorId: 1 }), 3); assert.equal(selectAuthor.getFullRunCountForParams({ authorId: 1 }), 1); assert.equal(selectAuthor.getSkippedRunCountForParams({ authorId: 1 }), 2); - assert.equal(selectRawAuthorData.getInvokeCountForParams(1), 3); + assert.equal(selectRawAuthorData.getInvokeCountForParams(1), 2); assert.equal(selectRawAuthorData.getFullRunCountForParams(1), 1); assert.equal(selectRawAuthorData.getPhantomRunCountForParams(1), 1); - assert.equal(selectRawAuthorData.getSkippedRunCountForParams(1), 1); + assert.equal(selectRawAuthorData.getSkippedRunCountForParams(1), 0); }); it('should return the author for a book', () => { diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js index ccad658..86ba014 100644 --- a/tests/overlapping-dependencies.js +++ b/tests/overlapping-dependencies.js @@ -16,8 +16,7 @@ const assert = chai.assert; // eslint-disable-line prefer-destructuring // @TODO: Is this useful enough to promote this to its own file, and possibly export? const assertCountsForParams = (selectorFn, params, expectedCounts) => { const actualCounts = selectorFn.getAllCountsForParams(params); - - const verboseErrorInfo = `Checking counts for ${selectorFn.displayName}(${params}): + const verboseErrorInfo = `Checking counts for ${selectorFn.displayName}(${selectorFn.createKeyFromParams(params)}): Expected: ${JSON.stringify(expectedCounts)} Actual: ${JSON.stringify(actualCounts)}\n`; @@ -25,7 +24,7 @@ const assertCountsForParams = (selectorFn, params, expectedCounts) => { if (!Object.hasOwnProperty.call(actualCounts, key)) { assert.fail(`${verboseErrorInfo}Invalid count type for assertCountsForParams: "${key}" not found`); } - assert.equal(actualCounts[key], expectedCounts[key], `${verboseErrorInfo}Count type "${key}" should match`); + assert.equal(actualCounts[key], expectedCounts[key], `${verboseErrorInfo} Count type "${key}" should match`); }); }; @@ -75,10 +74,12 @@ describe('Overlapping dependencies', () => { beforeEach(() => { // The selectors get recreated for each test, to reset their call counts. selectRawAppointmentData = createParameterizedRootSelector( - function _selectRawAppointmentData(state, authorId) { - return state.appointmentDataById[authorId]; + function _selectRawAppointmentData(state, appointmentId) { + return state.appointmentDataById[appointmentId]; + }, + { + performanceChecksEnabled: true, }, - { runLoggingEnabled: true } ); selectRawAppointmentIds = createParameterizedRootSelector( function _selectRawAppointmentIds(state) { @@ -86,6 +87,7 @@ describe('Overlapping dependencies', () => { }, { compareSelectorResults: COMPARISON_PRESETS.SHALLOW_EQUAL, + performanceChecksEnabled: true, }, ); @@ -99,7 +101,9 @@ describe('Overlapping dependencies', () => { dateObject: makeDateObjectForDay(rawAppointmentData.dayNum), }; }, - { runLoggingEnabled: true } + { + performanceChecksEnabled: true, + }, ); selectAllAppointments = createParameterizedSelector( @@ -109,6 +113,9 @@ describe('Overlapping dependencies', () => { appointmentId => selectAppointmentById({ appointmentId }), ); }, + { + performanceChecksEnabled: true, + }, ); selectAppointmentsForDay = createParameterizedSelector( @@ -117,6 +124,9 @@ describe('Overlapping dependencies', () => { const allAppointments = selectAllAppointments(); return filter(allAppointments, appointment => appointment.dayNum === dayNum); }, + { + performanceChecksEnabled: true, + }, ); selectAppointmentsForDayRange = createParameterizedSelector( function _selectAppointmentsForDay({ startDayNum, endDayNum }) { @@ -127,6 +137,9 @@ describe('Overlapping dependencies', () => { appointment => appointment.dayNum >= startDayNum && appointment.dayNum <= endDayNum, ); }, + { + performanceChecksEnabled: true, + }, ); }); @@ -154,5 +167,16 @@ describe('Overlapping dependencies', () => { fullRunCount: 1, skippedRunCount: 1, }); + + assertCountsForParams(selectRawAppointmentData, 2, { + invokeCount: 2, + fullRunCount: 1, + phantomRunCount: 1, + }); + assertCountsForParams(selectAppointmentById, { appointmentId: 2 }, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); }); }); From 47b7de5af18cae2d16ab6554111ba4eb0e19752d Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Mon, 10 Dec 2018 20:09:00 -0500 Subject: [PATCH 5/8] Placeholder in upgrading and expanding tests --- package-lock.json | 6 + package.json | 1 + src/parameterizedSelectorFactory.js | 2 - tests/overlapping-dependencies.js | 158 ++------------- .../premade-selectors/appointmentSelectors.js | 158 +++++++++++++++ .../appointmentSelectors.test.js | 182 ++++++++++++++++++ tests/util/assertCountsForParams.js | 28 +++ tests/util/index.js | 1 + 8 files changed, 390 insertions(+), 146 deletions(-) create mode 100644 tests/premade-selectors/appointmentSelectors.js create mode 100644 tests/premade-selectors/appointmentSelectors.test.js create mode 100644 tests/util/assertCountsForParams.js create mode 100644 tests/util/index.js diff --git a/package-lock.json b/package-lock.json index f9055c7..7023247 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2887,6 +2887,12 @@ "integrity": "sha512-uoxnT7PYpyEnsja+yX+7v49B7LXxmzDJ2JALqHH3oEGzpM2U1IGcbfnOr8Dt57z3B/UWs7/iAgPFbmye8m4I0g==", "dev": true }, + "immer": { + "version": "1.8.0", + "resolved": "https://registry.npmjs.org/immer/-/immer-1.8.0.tgz", + "integrity": "sha512-zOox8DNAunPeQgKwbAiwEUAHhZXtMPZo7VZ7m7h9cpQL1I35bAeaxMfwYyLEIt6RZUelFfsOBfG1GJu/iQNgxw==", + "dev": true + }, "imurmurhash": { "version": "0.1.4", "resolved": "https://registry.npmjs.org/imurmurhash/-/imurmurhash-0.1.4.tgz", diff --git a/package.json b/package.json index d2013d4..20c54a3 100644 --- a/package.json +++ b/package.json @@ -40,6 +40,7 @@ "eslint": "^5.2.0", "eslint-config-airbnb": "^17.0.0", "eslint-plugin-import": "^2.13.0", + "immer": "1.8.0", "mocha": "^5.2.0", "nyc": "^12.0.2" } diff --git a/src/parameterizedSelectorFactory.js b/src/parameterizedSelectorFactory.js index 88f8077..b5be006 100644 --- a/src/parameterizedSelectorFactory.js +++ b/src/parameterizedSelectorFactory.js @@ -473,8 +473,6 @@ const parameterizedSelectorFactory = (innerFn, overrideOptions = {}) => { // @TODO: Make overrideable options for these values if (resultRecord.invokeCount > 5 && resultRecord.fullRunCount > 0.75 * resultRecord.invokeCount) { options.performanceChecksCallback(`${loggingPrefix} is recomputing a lot: ${resultRecord.fullRunCount} of ${resultRecord.invokeCount} runs gave new results.`); - } else if (globalInvokeCount > 25 && globalFullRunCount > 0.75 * globalInvokeCount) { - options.performanceChecksCallback(`${options.displayName} is recomputing a lot in total: ${globalFullRunCount} of ${globalInvokeCount} runs gave new results.`); } } } diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js index 86ba014..148c44a 100644 --- a/tests/overlapping-dependencies.js +++ b/tests/overlapping-dependencies.js @@ -1,13 +1,6 @@ /* eslint-env mocha */ -/* eslint-disable prefer-arrow-callback */ import chai from 'chai'; -import filter from 'lodash/filter'; - -import { - COMPARISON_PRESETS, - createParameterizedRootSelector, - createParameterizedSelector, -} from '../src/index'; +import { getInitialState, getSelectors } from './premade-selectors/appointmentSelectors'; const assert = chai.assert; // eslint-disable-line prefer-destructuring @@ -30,153 +23,30 @@ const assertCountsForParams = (selectorFn, params, expectedCounts) => { describe('Overlapping dependencies', () => { - const initialState = { - appointmentDataById: { - 0: { dayNum: 0, title: 'Start of semester' }, - 1: { dayNum: 1, title: 'Orientation' }, - 2: { dayNum: 5, title: 'Classes begin' }, - 3: { dayNum: 50, title: 'Midterms, day 1' }, - 4: { dayNum: 51, title: 'Midterms, day 2' }, - 5: { dayNum: 95, title: 'Finals, day 1' }, - 6: { dayNum: 96, title: 'Finals, day 2' }, - 7: { dayNum: 97, title: 'Finals, day 3' }, - 8: { dayNum: 99, title: 'End of semester' }, - 20: { dayNum: 10, title: 'Day 10' }, - 21: { dayNum: 20, title: 'Day 20' }, - 22: { dayNum: 30, title: 'Day 30' }, - 23: { dayNum: 40, title: 'Day 40' }, - 24: { dayNum: 50, title: 'Day 50' }, - 25: { dayNum: 60, title: 'Day 60' }, - 26: { dayNum: 70, title: 'Day 70' }, - 27: { dayNum: 80, title: 'Day 80' }, - 28: { dayNum: 90, title: 'Day 90' }, - 40: { dayNum: 15, title: 'Alice\'s birthday' }, - 41: { dayNum: 25, title: 'Bob\'s birthday' }, - 42: { dayNum: 41, title: 'Chris\'s birthday' }, - 43: { dayNum: 89, title: 'Alice & Bob\'s anniversary' }, - 44: { dayNum: 11, title: 'Break' }, - 45: { dayNum: 12, title: 'Break' }, - 46: { dayNum: 13, title: 'Break' }, - 47: { dayNum: 75, title: 'Vacation' }, - 48: { dayNum: 76, title: 'Vacation' }, - 49: { dayNum: 77, title: 'Vacation' }, - }, - }; - const makeDateObjectForDay = dayNum => new Date(2018, 0, dayNum); + const initialState = getInitialState(); let selectRawAppointmentData; let selectRawAppointmentIds; let selectAppointmentById; let selectAllAppointments; + let selectAllAppointmentsInOrder; let selectAppointmentsForDay; let selectAppointmentsForDayRange; + let selectAppointmentsForDayRangeInOrder; beforeEach(() => { // The selectors get recreated for each test, to reset their call counts. - selectRawAppointmentData = createParameterizedRootSelector( - function _selectRawAppointmentData(state, appointmentId) { - return state.appointmentDataById[appointmentId]; - }, - { - performanceChecksEnabled: true, - }, - ); - selectRawAppointmentIds = createParameterizedRootSelector( - function _selectRawAppointmentIds(state) { - return Object.keys(state.appointmentDataById); - }, - { - compareSelectorResults: COMPARISON_PRESETS.SHALLOW_EQUAL, - performanceChecksEnabled: true, - }, - ); - - // This extra layer represents a transformation or conversion from serializable Redux data into something - // with rich objects, e.g. if you're using a date library. - selectAppointmentById = createParameterizedSelector( - function _selectAppointmentById({ appointmentId }) { - const rawAppointmentData = selectRawAppointmentData(appointmentId); - return { - ...rawAppointmentData, - dateObject: makeDateObjectForDay(rawAppointmentData.dayNum), - }; - }, - { - performanceChecksEnabled: true, - }, - ); - - selectAllAppointments = createParameterizedSelector( - function _selectAllAppointments() { - const rawAppointmentIds = selectRawAppointmentIds(); - return rawAppointmentIds.map( - appointmentId => selectAppointmentById({ appointmentId }), - ); - }, - { - performanceChecksEnabled: true, - }, - ); - - selectAppointmentsForDay = createParameterizedSelector( - function _selectAppointmentsForDay({ dayNum }) { - // This naive implementation is going to walk through all appointments - const allAppointments = selectAllAppointments(); - return filter(allAppointments, appointment => appointment.dayNum === dayNum); - }, - { - performanceChecksEnabled: true, - }, - ); - selectAppointmentsForDayRange = createParameterizedSelector( - function _selectAppointmentsForDay({ startDayNum, endDayNum }) { - // This naive implementation is going to walk through all appointments - const allAppointments = selectAllAppointments(); - return filter( - allAppointments, - appointment => appointment.dayNum >= startDayNum && appointment.dayNum <= endDayNum, - ); - }, - { - performanceChecksEnabled: true, - }, - ); + ({ + selectRawAppointmentData, + selectRawAppointmentIds, + selectAppointmentById, + selectAllAppointments, + selectAllAppointmentsInOrder, + selectAppointmentsForDay, + selectAppointmentsForDayRange, + selectAppointmentsForDayRangeInOrder, + } = getSelectors()); }); - // First, some tests to ensure the selectors work as expected in general. - - it('should return appointment models by ID', () => { - const appointment1 = selectAppointmentById(initialState, { appointmentId: 1 }); - const appointment2 = selectAppointmentById(initialState, { appointmentId: 2 }); - assert.equal(appointment1.title, 'Orientation'); - assert.equal(appointment2.title, 'Classes begin'); - - // No changes after a no-change state change - const newState = { ...initialState }; - assert.equal(appointment1, selectAppointmentById(newState, { appointmentId: 1 })); - assert.equal(appointment2, selectAppointmentById(newState, { appointmentId: 2 })); - - assertCountsForParams(selectRawAppointmentData, 1, { - invokeCount: 2, - fullRunCount: 1, - phantomRunCount: 1, - }); - assertCountsForParams(selectAppointmentById, { appointmentId: 1 }, { - invokeCount: 2, - fullRunCount: 1, - skippedRunCount: 1, - }); - - assertCountsForParams(selectRawAppointmentData, 2, { - invokeCount: 2, - fullRunCount: 1, - phantomRunCount: 1, - }); - assertCountsForParams(selectAppointmentById, { appointmentId: 2 }, { - invokeCount: 2, - fullRunCount: 1, - skippedRunCount: 1, - }); - }); }); diff --git a/tests/premade-selectors/appointmentSelectors.js b/tests/premade-selectors/appointmentSelectors.js new file mode 100644 index 0000000..c4599b1 --- /dev/null +++ b/tests/premade-selectors/appointmentSelectors.js @@ -0,0 +1,158 @@ +/* eslint-disable prefer-arrow-callback */ + +import sortBy from 'lodash/sortBy'; +import filter from 'lodash/filter'; + +import { + COMPARISON_PRESETS, + createParameterizedRootSelector, + createParameterizedSelector, +} from '../../src'; + + +const makeDateObjectForDay = dayNum => new Date(2018, 0, dayNum); + +const getInitialState = () => ({ + appointmentDataById: { + 0: { dayNum: 0, title: 'Start of semester' }, + 1: { dayNum: 1, title: 'Orientation' }, + 2: { dayNum: 5, title: 'Classes begin' }, + 3: { dayNum: 50, title: 'Midterms, day 1' }, + 4: { dayNum: 51, title: 'Midterms, day 2' }, + 5: { dayNum: 95, title: 'Finals, day 1' }, + 6: { dayNum: 96, title: 'Finals, day 2' }, + 7: { dayNum: 97, title: 'Finals, day 3' }, + 8: { dayNum: 99, title: 'End of semester' }, + 20: { dayNum: 10, title: 'Day 10' }, + 21: { dayNum: 20, title: 'Day 20' }, + 22: { dayNum: 30, title: 'Day 30' }, + 23: { dayNum: 40, title: 'Day 40' }, + 24: { dayNum: 50, title: 'Day 50' }, + 25: { dayNum: 60, title: 'Day 60' }, + 26: { dayNum: 70, title: 'Day 70' }, + 27: { dayNum: 80, title: 'Day 80' }, + 28: { dayNum: 90, title: 'Day 90' }, + 40: { dayNum: 10, title: 'Alice\'s birthday' }, + 41: { dayNum: 20, title: 'Bob\'s birthday' }, + 42: { dayNum: 40, title: 'Chris\'s birthday' }, + 43: { dayNum: 80, title: 'Alice & Bob\'s anniversary' }, + 44: { dayNum: 10, title: 'Break' }, + 45: { dayNum: 11, title: 'Break' }, + 46: { dayNum: 12, title: 'Break' }, + 47: { dayNum: 77, title: 'Vacation' }, + 48: { dayNum: 71, title: 'Vacation' }, + 49: { dayNum: 72, title: 'Vacation' }, + }, +}); + + +const getSelectors = () => { + const selectRawAppointmentData = createParameterizedRootSelector( + function _selectRawAppointmentData(state, appointmentId) { + return state.appointmentDataById[appointmentId]; + }, + { + performanceChecksEnabled: true, + }, + ); + + const selectRawAppointmentIds = createParameterizedRootSelector( + function _selectRawAppointmentIds(state) { + return Object.keys(state.appointmentDataById); + }, + { + compareSelectorResults: COMPARISON_PRESETS.SHALLOW_EQUAL, + performanceChecksEnabled: true, + }, + ); + + // This extra layer represents a transformation or conversion from serializable Redux data into something + // with rich objects, e.g. if you're using a date library. + const selectAppointmentById = createParameterizedSelector( + function _selectAppointmentById({ appointmentId }) { + const rawAppointmentData = selectRawAppointmentData(appointmentId); + return { + ...rawAppointmentData, + dateObject: makeDateObjectForDay(rawAppointmentData.dayNum), + }; + }, + { + performanceChecksEnabled: true, + }, + ); + + const selectAllAppointments = createParameterizedSelector( + function _selectAllAppointments() { + const rawAppointmentIds = selectRawAppointmentIds(); + return rawAppointmentIds.map( + appointmentId => selectAppointmentById({ appointmentId }), + ); + }, + { + performanceChecksEnabled: true, + }, + ); + + const selectAllAppointmentsInOrder = createParameterizedSelector( + function _selectAllAppointmentsInOrder() { + const allAppointments = selectAllAppointments(); + return sortBy(allAppointments, 'day'); + }, + { + performanceChecksEnabled: true, + }, + ); + + const selectAppointmentsForDay = createParameterizedSelector( + function _selectAppointmentsForDay({ dayNum }) { + // This naive implementation is going to walk through all appointments + const allAppointments = selectAllAppointments(); + return filter(allAppointments, appointment => appointment.dayNum === dayNum); + }, + { + performanceChecksEnabled: true, + }, + ); + + const selectAppointmentsForDayRange = createParameterizedSelector( + function _selectAppointmentsForDay({ startDayNum, endDayNum }) { + // This naive implementation is going to walk through all appointments + const allAppointments = selectAllAppointments(); + return filter( + allAppointments, + appointment => appointment.dayNum >= startDayNum && appointment.dayNum <= endDayNum, + ); + }, + { + performanceChecksEnabled: true, + }, + ); + + const selectAppointmentsForDayRangeInOrder = createParameterizedSelector( + function _selectAppointmentsForDayRangeInOrder({ startDayNum, endDayNum }) { + const appointmentsInRange = selectAppointmentsForDayRange({ startDayNum, endDayNum }); + return sortBy(appointmentsInRange, 'day'); + }, + { + performanceChecksEnabled: true, + }, + ); + + return { + selectRawAppointmentData, + selectRawAppointmentIds, + selectAppointmentById, + selectAllAppointments, + selectAllAppointmentsInOrder, + selectAppointmentsForDay, + selectAppointmentsForDayRange, + selectAppointmentsForDayRangeInOrder, + }; +}; + + +export { + getInitialState, + getSelectors, + makeDateObjectForDay, +}; diff --git a/tests/premade-selectors/appointmentSelectors.test.js b/tests/premade-selectors/appointmentSelectors.test.js new file mode 100644 index 0000000..76701ff --- /dev/null +++ b/tests/premade-selectors/appointmentSelectors.test.js @@ -0,0 +1,182 @@ +/* eslint-env mocha */ +import chai from 'chai'; +import produce from 'immer'; + +import { getInitialState, getSelectors } from './appointmentSelectors'; +import { assertCountsForParams } from '../util'; + +const { assert } = chai; + + +describe('Premade appointment selectors', () => { + let initialState; + + let selectRawAppointmentData; + let selectRawAppointmentIds; + let selectAppointmentById; + let selectAllAppointments; + let selectAllAppointmentsInOrder; + let selectAppointmentsForDay; + let selectAppointmentsForDayRange; + let selectAppointmentsForDayRangeInOrder; + + beforeEach(() => { + initialState = getInitialState(); + + // The selectors get recreated for each test, to reset their call counts. + ({ + selectRawAppointmentData, + selectRawAppointmentIds, + selectAppointmentById, + selectAllAppointments, + selectAllAppointmentsInOrder, + selectAppointmentsForDay, + selectAppointmentsForDayRange, + selectAppointmentsForDayRangeInOrder, + } = getSelectors()); + }); + + + it('selectRawAppointmentData', () => { + let state = initialState; + const appointment1_1 = selectRawAppointmentData(state, 1); + const appointment2_1 = selectRawAppointmentData(state, 2); + + assert.equal(appointment1_1.title, 'Orientation'); + assert.equal(tests/premade-selectors/appointmentSelectors.test.js:46.title, 'Classes begin'); + assertCountsForParams(selectRawAppointmentData, 1, { + invokeCount: 1, + fullRunCount: 1, + }); + + // A no-impact change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById = state.appointmentDataById; + }); + + const appointment1_2 = selectRawAppointmentData(state, 1); + const appointment2_2 = selectRawAppointmentData(state, 2); + + assert.equal(appointment1_2, appointment1_1); + assert.equal(appointment2_2, appointment2_1); + assertCountsForParams(selectRawAppointmentData, 1, { + invokeCount: 2, + fullRunCount: 1, + phantomRunCount: 1, + }); + + // An impactful change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById[1].title = 'Welcome'; + // eslint-disable-next-line no-self-assign + newState.appointmentDataById[2].title = state.appointmentDataById[2].title; + }); + + const appointment1_3 = selectRawAppointmentData(state, 1); + const appointment2_3 = selectRawAppointmentData(state, 2); + + assert.equal(appointment1_3.title, 'Welcome'); + assert.notEqual(appointment1_3, appointment1_1); + assert.equal(appointment2_3.title, 'Classes begin'); + assert.equal(appointment2_3, appointment2_1); + assertCountsForParams(selectRawAppointmentData, 1, { + invokeCount: 3, + fullRunCount: 2, + phantomRunCount: 1, + }); + assertCountsForParams(selectRawAppointmentData, 2, { + invokeCount: 3, + fullRunCount: 1, + phantomRunCount: 2, + }); + }); + + + it('selectRawAppointmentIds', () => { + let state = initialState; + let allAppointmentIds = selectRawAppointmentIds(); + + assert.equal(allAppointmentIds[10], 21); + assert.equal(allAppointmentIds[11], 22); + assertCountsForParams(selectRawAppointmentIds, null, { + invokeCount: 1, + fullRunCount: 1, + }); + + // An impactful change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById[50] = { + dayNum: 50, + title: 'New event!', + }; + }); + + allAppointmentIds = selectRawAppointmentIds(); + + assert.equal(allAppointmentIds[10], 21); + assert.equal(allAppointmentIds[11], 22); + assertCountsForParams(selectRawAppointmentIds, null, { + invokeCount: 2, + fullRunCount: 2, + }); + + // A no-impact change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById = state.appointmentDataById; + }); + + allAppointmentIds = selectRawAppointmentIds(); + + assert.equal(allAppointmentIds[10], 21); + assert.equal(allAppointmentIds[11], 22); + assertCountsForParams(selectRawAppointmentIds, null, { + invokeCount: 3, + fullRunCount: 2, + phantomRunCount: 1, + }); + }); + + + it('selects appointment models by ID', () => { + let state = initialState; + const appointment1 = selectAppointmentById(initialState, { appointmentId: 1 }); + const appointment2 = selectAppointmentById(initialState, { appointmentId: 2 }); + + assert.equal(appointment1.title, 'Orientation'); + assert.equal(appointment2.title, 'Classes begin'); + + assertCountsForParams(selectAppointmentById, { appointmentId: 1 }, { + invokeCount: 1, + fullRunCount: 1, + }); + + // A no-impact change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById = state.appointmentDataById; + }); + + assert.equal(appointment1, selectAppointmentById(state, { appointmentId: 1 })); + assert.equal(appointment2, selectAppointmentById(state, { appointmentId: 2 })); + + assertCountsForParams(selectRawAppointmentData, 1, { + invokeCount: 2, + fullRunCount: 1, + phantomRunCount: 1, + }); + assertCountsForParams(selectAppointmentById, { appointmentId: 1 }, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); + }); + + it('should return all appointment models', () => { + const appointmentList = selectAllAppointments(initialState); + + }); +}); diff --git a/tests/util/assertCountsForParams.js b/tests/util/assertCountsForParams.js new file mode 100644 index 0000000..d0b48df --- /dev/null +++ b/tests/util/assertCountsForParams.js @@ -0,0 +1,28 @@ +/* eslint-env mocha */ +import chai from 'chai'; + +const { assert } = chai; + + +/** + * This is basically an implementation of assert.deepInclude but with better error messages. + * + * @param {Function} selectorFn + * @param {Object|String|Number} params + * @param {Object} expectedCounts + */ +const assertCountsForParams = (selectorFn, params, expectedCounts) => { + const actualCounts = selectorFn.getAllCountsForParams(params); + const verboseErrorInfo = `Checking counts for ${selectorFn.displayName}(${selectorFn.createKeyFromParams(params)}): + Expected: ${JSON.stringify(expectedCounts)} + Actual: ${JSON.stringify(actualCounts)}\n`; + + Object.keys(expectedCounts).forEach((key) => { + if (!Object.hasOwnProperty.call(actualCounts, key)) { + assert.fail(`${verboseErrorInfo}Invalid count type for assertCountsForParams: "${key}" not found`); + } + assert.equal(actualCounts[key], expectedCounts[key], `${verboseErrorInfo} Count type "${key}" should match`); + }); +}; + +export default assertCountsForParams; diff --git a/tests/util/index.js b/tests/util/index.js new file mode 100644 index 0000000..df642fa --- /dev/null +++ b/tests/util/index.js @@ -0,0 +1 @@ +export assertCountsForParams from './assertCountsForParams'; From 3289706c2b9599c5a6f6c2e8e495b03bfebf526d Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Mon, 10 Dec 2018 22:11:42 -0500 Subject: [PATCH 6/8] Expand tests and make them more robust --- package.json | 2 +- src/helpers.js | 3 + tests/overlapping-dependencies.js | 111 +++++++++++++++--- .../premade-selectors/appointmentSelectors.js | 4 +- .../appointmentSelectors.test.js | 59 +++++++--- 5 files changed, 140 insertions(+), 39 deletions(-) diff --git a/package.json b/package.json index 20c54a3..35494b0 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ "url": "git+https://github.com/spautz/parameterized-selectors.git" }, "scripts": { - "test": "nyc mocha --require babel-core/register tests/", + "test": "nyc mocha --require babel-core/register tests/*", "lint": "eslint .", "sendCoverage": "nyc report --reporter=text-lcov | coveralls" }, diff --git a/src/helpers.js b/src/helpers.js index 9c511c3..d0a62a3 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -75,6 +75,9 @@ const KEY_PRESETS = { if (obj && typeof obj === 'object') { return JSON.stringify(obj); } + if (obj == null) { + return '{}'; + } return String(obj); }, JSON_STRING_WITH_STABLE_KEYS: (obj) => { diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js index 148c44a..c168220 100644 --- a/tests/overlapping-dependencies.js +++ b/tests/overlapping-dependencies.js @@ -1,29 +1,16 @@ /* eslint-env mocha */ +/* eslint-disable camelcase */ import chai from 'chai'; +import produce from 'immer'; + import { getInitialState, getSelectors } from './premade-selectors/appointmentSelectors'; +import { assertCountsForParams } from './util'; const assert = chai.assert; // eslint-disable-line prefer-destructuring -// This is basically an implementation of assert.deepInclude but with better error messages. -// @TODO: Is this useful enough to promote this to its own file, and possibly export? -const assertCountsForParams = (selectorFn, params, expectedCounts) => { - const actualCounts = selectorFn.getAllCountsForParams(params); - const verboseErrorInfo = `Checking counts for ${selectorFn.displayName}(${selectorFn.createKeyFromParams(params)}): - Expected: ${JSON.stringify(expectedCounts)} - Actual: ${JSON.stringify(actualCounts)}\n`; - - Object.keys(expectedCounts).forEach(function (key) { - if (!Object.hasOwnProperty.call(actualCounts, key)) { - assert.fail(`${verboseErrorInfo}Invalid count type for assertCountsForParams: "${key}" not found`); - } - assert.equal(actualCounts[key], expectedCounts[key], `${verboseErrorInfo} Count type "${key}" should match`); - }); -}; - - describe('Overlapping dependencies', () => { - const initialState = getInitialState(); + let initialState; let selectRawAppointmentData; let selectRawAppointmentIds; @@ -35,6 +22,8 @@ describe('Overlapping dependencies', () => { let selectAppointmentsForDayRangeInOrder; beforeEach(() => { + initialState = getInitialState(); + // The selectors get recreated for each test, to reset their call counts. ({ selectRawAppointmentData, @@ -49,4 +38,90 @@ describe('Overlapping dependencies', () => { }); + it('works properly when selecting outer selector before inner', () => { + let state = initialState; + const allAppointmentsInOrder_1 = selectAllAppointmentsInOrder(state); + const allAppointments_1 = selectAllAppointments(state); + + assert.equal(allAppointmentsInOrder_1[5].title, 'Break'); + assert.equal(allAppointments_1[5].title, 'Finals, day 1'); + assertCountsForParams(selectRawAppointmentData, 5, { + invokeCount: 1, + fullRunCount: 1, + }); + assertCountsForParams(selectAllAppointmentsInOrder, null, { + invokeCount: 1, + fullRunCount: 1, + }); + assertCountsForParams(selectAllAppointments, null, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); + + // A no-impact change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById = { ...state.appointmentDataById }; + }); + + const allAppointmentsInOrder_2 = selectAllAppointmentsInOrder(state); + const allAppointments_2 = selectAllAppointments(state); + + assert.equal(allAppointmentsInOrder_2[5].title, 'Break'); + assert.equal(allAppointments_2[5].title, 'Finals, day 1'); + assertCountsForParams(selectRawAppointmentData, 5, { + invokeCount: 3, + fullRunCount: 1, + phantomRunCount: 1, + skippedRunCount: 1, + }); + assertCountsForParams(selectAllAppointmentsInOrder, null, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); + assertCountsForParams(selectAllAppointments, null, { + invokeCount: 3, + fullRunCount: 1, + skippedRunCount: 2, + }); + + // A second no-impact change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById[45].title = 'Break (day 2)'; + }); + + const allAppointmentsInOrder_3 = selectAllAppointmentsInOrder(state); + const allAppointments_3 = selectAllAppointments(state); + + assert.equal(allAppointmentsInOrder_3[5].title, 'Break'); + assert.equal(allAppointments_3[5].title, 'Finals, day 1'); + assertCountsForParams(selectRawAppointmentData, 5, { + invokeCount: 6, // @FIXME: This value looks wrong: should it be 5? + fullRunCount: 1, + phantomRunCount: 2, + skippedRunCount: 3, + }); + assertCountsForParams(selectAllAppointmentsInOrder, null, { + invokeCount: 3, + fullRunCount: 2, + skippedRunCount: 1, + }); + assertCountsForParams(selectAllAppointments, null, { + invokeCount: 6, + fullRunCount: 2, + skippedRunCount: 4, + }); + }); + + it('works properly when selecting outer selector before inner, with a different selector', () => { + }); + + it('works properly when selecting inner selector before outer', () => { + }); + + it('works properly when selecting inner selector before outer, with a different selector', () => { + }); }); diff --git a/tests/premade-selectors/appointmentSelectors.js b/tests/premade-selectors/appointmentSelectors.js index c4599b1..60e190e 100644 --- a/tests/premade-selectors/appointmentSelectors.js +++ b/tests/premade-selectors/appointmentSelectors.js @@ -96,7 +96,7 @@ const getSelectors = () => { const selectAllAppointmentsInOrder = createParameterizedSelector( function _selectAllAppointmentsInOrder() { const allAppointments = selectAllAppointments(); - return sortBy(allAppointments, 'day'); + return sortBy(allAppointments, 'dayNum'); }, { performanceChecksEnabled: true, @@ -131,7 +131,7 @@ const getSelectors = () => { const selectAppointmentsForDayRangeInOrder = createParameterizedSelector( function _selectAppointmentsForDayRangeInOrder({ startDayNum, endDayNum }) { const appointmentsInRange = selectAppointmentsForDayRange({ startDayNum, endDayNum }); - return sortBy(appointmentsInRange, 'day'); + return sortBy(appointmentsInRange, 'dayNum'); }, { performanceChecksEnabled: true, diff --git a/tests/premade-selectors/appointmentSelectors.test.js b/tests/premade-selectors/appointmentSelectors.test.js index 76701ff..cbbcc9a 100644 --- a/tests/premade-selectors/appointmentSelectors.test.js +++ b/tests/premade-selectors/appointmentSelectors.test.js @@ -1,4 +1,5 @@ /* eslint-env mocha */ +/* eslint-disable camelcase */ import chai from 'chai'; import produce from 'immer'; @@ -15,10 +16,10 @@ describe('Premade appointment selectors', () => { let selectRawAppointmentIds; let selectAppointmentById; let selectAllAppointments; - let selectAllAppointmentsInOrder; - let selectAppointmentsForDay; - let selectAppointmentsForDayRange; - let selectAppointmentsForDayRangeInOrder; + let selectAllAppointmentsInOrder; // eslint-disable-line no-unused-vars, @TODO: More tests + let selectAppointmentsForDay; // eslint-disable-line no-unused-vars, @TODO: More tests + let selectAppointmentsForDayRange; // eslint-disable-line no-unused-vars, @TODO: More tests + let selectAppointmentsForDayRangeInOrder; // eslint-disable-line no-unused-vars, @TODO: More tests beforeEach(() => { initialState = getInitialState(); @@ -43,7 +44,7 @@ describe('Premade appointment selectors', () => { const appointment2_1 = selectRawAppointmentData(state, 2); assert.equal(appointment1_1.title, 'Orientation'); - assert.equal(tests/premade-selectors/appointmentSelectors.test.js:46.title, 'Classes begin'); + assert.equal(appointment2_1.title, 'Classes begin'); assertCountsForParams(selectRawAppointmentData, 1, { invokeCount: 1, fullRunCount: 1, @@ -52,7 +53,7 @@ describe('Premade appointment selectors', () => { // A no-impact change state = produce(state, (newState) => { /* eslint-disable no-param-reassign */ - newState.appointmentDataById = state.appointmentDataById; + newState.appointmentDataById = { ...state.appointmentDataById }; }); const appointment1_2 = selectRawAppointmentData(state, 1); @@ -96,7 +97,7 @@ describe('Premade appointment selectors', () => { it('selectRawAppointmentIds', () => { let state = initialState; - let allAppointmentIds = selectRawAppointmentIds(); + let allAppointmentIds = selectRawAppointmentIds(state); assert.equal(allAppointmentIds[10], 21); assert.equal(allAppointmentIds[11], 22); @@ -114,7 +115,7 @@ describe('Premade appointment selectors', () => { }; }); - allAppointmentIds = selectRawAppointmentIds(); + allAppointmentIds = selectRawAppointmentIds(state); assert.equal(allAppointmentIds[10], 21); assert.equal(allAppointmentIds[11], 22); @@ -126,10 +127,10 @@ describe('Premade appointment selectors', () => { // A no-impact change state = produce(state, (newState) => { /* eslint-disable no-param-reassign */ - newState.appointmentDataById = state.appointmentDataById; + newState.appointmentDataById = { ...state.appointmentDataById }; }); - allAppointmentIds = selectRawAppointmentIds(); + allAppointmentIds = selectRawAppointmentIds(state); assert.equal(allAppointmentIds[10], 21); assert.equal(allAppointmentIds[11], 22); @@ -143,11 +144,11 @@ describe('Premade appointment selectors', () => { it('selects appointment models by ID', () => { let state = initialState; - const appointment1 = selectAppointmentById(initialState, { appointmentId: 1 }); - const appointment2 = selectAppointmentById(initialState, { appointmentId: 2 }); + const appointment1_1 = selectAppointmentById(state, { appointmentId: 1 }); + const appointment2_1 = selectAppointmentById(state, { appointmentId: 2 }); - assert.equal(appointment1.title, 'Orientation'); - assert.equal(appointment2.title, 'Classes begin'); + assert.equal(appointment1_1.title, 'Orientation'); + assert.equal(appointment2_1.title, 'Classes begin'); assertCountsForParams(selectAppointmentById, { appointmentId: 1 }, { invokeCount: 1, @@ -157,11 +158,14 @@ describe('Premade appointment selectors', () => { // A no-impact change state = produce(state, (newState) => { /* eslint-disable no-param-reassign */ - newState.appointmentDataById = state.appointmentDataById; + newState.appointmentDataById = { ...state.appointmentDataById }; }); - assert.equal(appointment1, selectAppointmentById(state, { appointmentId: 1 })); - assert.equal(appointment2, selectAppointmentById(state, { appointmentId: 2 })); + const appointment1_2 = selectAppointmentById(state, { appointmentId: 1 }); + const appointment2_2 = selectAppointmentById(state, { appointmentId: 2 }); + + assert.equal(appointment1_2, appointment1_1); + assert.equal(appointment2_2, appointment2_1); assertCountsForParams(selectRawAppointmentData, 1, { invokeCount: 2, @@ -173,10 +177,29 @@ describe('Premade appointment selectors', () => { fullRunCount: 1, skippedRunCount: 1, }); + + // An impactful change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById[1].title = 'Welcome'; + // eslint-disable-next-line no-self-assign + newState.appointmentDataById[2].title = state.appointmentDataById[2].title; + }); }); it('should return all appointment models', () => { - const appointmentList = selectAllAppointments(initialState); + let state = initialState; + const appointmentList_1 = selectAllAppointments(state); + assert.equal(appointmentList_1[1].title, 'Orientation'); + + // An impactful change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById[1].title = 'Welcome'; + }); + const appointmentList_2 = selectAllAppointments(state); + assert.equal(appointmentList_2[1].title, 'Welcome'); + assert.equal(appointmentList_2[2].title, 'Classes begin'); }); }); From ca9bf6adcb0511a48d66d29e50327ccf9ca1b286 Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Mon, 10 Dec 2018 22:16:33 -0500 Subject: [PATCH 7/8] Expand tests further --- tests/overlapping-dependencies.js | 77 +++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-) diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js index c168220..9c1630f 100644 --- a/tests/overlapping-dependencies.js +++ b/tests/overlapping-dependencies.js @@ -116,12 +116,81 @@ describe('Overlapping dependencies', () => { }); }); - it('works properly when selecting outer selector before inner, with a different selector', () => { - }); it('works properly when selecting inner selector before outer', () => { - }); + let state = initialState; + const allAppointments_1 = selectAllAppointments(state); + const allAppointmentsInOrder_1 = selectAllAppointmentsInOrder(state); - it('works properly when selecting inner selector before outer, with a different selector', () => { + assert.equal(allAppointmentsInOrder_1[5].title, 'Break'); + assert.equal(allAppointments_1[5].title, 'Finals, day 1'); + assertCountsForParams(selectRawAppointmentData, 5, { + invokeCount: 1, + fullRunCount: 1, + }); + assertCountsForParams(selectAllAppointmentsInOrder, null, { + invokeCount: 1, + fullRunCount: 1, + }); + assertCountsForParams(selectAllAppointments, null, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); + + // A no-impact change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById = { ...state.appointmentDataById }; + }); + + const allAppointments_2 = selectAllAppointments(state); + const allAppointmentsInOrder_2 = selectAllAppointmentsInOrder(state); + + assert.equal(allAppointmentsInOrder_2[5].title, 'Break'); + assert.equal(allAppointments_2[5].title, 'Finals, day 1'); + assertCountsForParams(selectRawAppointmentData, 5, { + invokeCount: 2, + fullRunCount: 1, + phantomRunCount: 1, + }); + assertCountsForParams(selectAllAppointmentsInOrder, null, { + invokeCount: 2, + fullRunCount: 1, + skippedRunCount: 1, + }); + assertCountsForParams(selectAllAppointments, null, { + invokeCount: 3, + fullRunCount: 1, + skippedRunCount: 2, + }); + + // A second no-impact change + state = produce(state, (newState) => { + /* eslint-disable no-param-reassign */ + newState.appointmentDataById[45].title = 'Break (day 2)'; + }); + + const allAppointments_3 = selectAllAppointments(state); + const allAppointmentsInOrder_3 = selectAllAppointmentsInOrder(state); + + assert.equal(allAppointmentsInOrder_3[5].title, 'Break'); + assert.equal(allAppointments_3[5].title, 'Finals, day 1'); + assertCountsForParams(selectRawAppointmentData, 5, { + invokeCount: 4, + fullRunCount: 1, + phantomRunCount: 2, + skippedRunCount: 1, + }); + assertCountsForParams(selectAllAppointmentsInOrder, null, { + invokeCount: 3, + fullRunCount: 1, + skippedRunCount: 2, + }); + assertCountsForParams(selectAllAppointments, null, { + invokeCount: 4, + fullRunCount: 2, + skippedRunCount: 2, + }); }); }); From ea7f79285d4b2576f17dc18da910fb79304a351c Mon Sep 17 00:00:00 2001 From: Steven Pautz Date: Mon, 10 Dec 2018 22:30:03 -0500 Subject: [PATCH 8/8] Fix linter errors --- tests/overlapping-dependencies.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/overlapping-dependencies.js b/tests/overlapping-dependencies.js index 9c1630f..90a74eb 100644 --- a/tests/overlapping-dependencies.js +++ b/tests/overlapping-dependencies.js @@ -13,13 +13,13 @@ describe('Overlapping dependencies', () => { let initialState; let selectRawAppointmentData; - let selectRawAppointmentIds; - let selectAppointmentById; + // let selectRawAppointmentIds; + // let selectAppointmentById; let selectAllAppointments; let selectAllAppointmentsInOrder; - let selectAppointmentsForDay; - let selectAppointmentsForDayRange; - let selectAppointmentsForDayRangeInOrder; + // let selectAppointmentsForDay; + // let selectAppointmentsForDayRange; + // let selectAppointmentsForDayRangeInOrder; beforeEach(() => { initialState = getInitialState(); @@ -27,13 +27,13 @@ describe('Overlapping dependencies', () => { // The selectors get recreated for each test, to reset their call counts. ({ selectRawAppointmentData, - selectRawAppointmentIds, - selectAppointmentById, + // selectRawAppointmentIds, + // selectAppointmentById, selectAllAppointments, selectAllAppointmentsInOrder, - selectAppointmentsForDay, - selectAppointmentsForDayRange, - selectAppointmentsForDayRangeInOrder, + // selectAppointmentsForDay, + // selectAppointmentsForDayRange, + // selectAppointmentsForDayRangeInOrder, } = getSelectors()); });