Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: remove immer from static report #501

Merged
merged 1 commit into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions lib/static/modules/reducers/api-values.js
Original file line number Diff line number Diff line change
@@ -1,25 +1,16 @@
import actionNames from '../action-names';
import {applyStateUpdate} from '../utils/state';

export default (state, action) => {
switch (action.type) {
case actionNames.INIT_GUI_REPORT:
case actionNames.INIT_STATIC_REPORT: {
const {apiValues} = action.payload;

return applyChanges(state, apiValues);
return applyStateUpdate(state, {apiValues});
Copy link
Member

Choose a reason for hiding this comment

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

А почему просто не назвать updateState?

Copy link
Member Author

Choose a reason for hiding this comment

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

потому что state не обновляется. Он иммутабелен, и updateState по сути ничего обновлять не будет. Тут именно создается новый стейт из старого и диффа

}

default:
return state;
}
};

function applyChanges(state, apiValues) {
return {
...state,
apiValues: {
...state.apiValues,
...apiValues
}
};
}
10 changes: 4 additions & 6 deletions lib/static/modules/reducers/bottom-progress-bar.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import actionNames from '../action-names';
import {produce} from 'immer';
import {set} from 'lodash';
import {applyStateUpdate} from '../utils/state';

export default produce((draft, action) => {
export default ((state, action) => {
switch (action.type) {
case actionNames.UPDATE_BOTTOM_PROGRESS_BAR: {
const {currentRootSuiteId} = action.payload;

return set(draft, 'progressBar.currentRootSuiteId', currentRootSuiteId);
return applyStateUpdate(state, {progressBar: {currentRootSuiteId}});
}

default:
return draft;
return state;
}
});
15 changes: 3 additions & 12 deletions lib/static/modules/reducers/config.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {cloneDeep} from 'lodash';
import {CONTROL_TYPE_RADIOBUTTON} from '../../../gui/constants/custom-gui-control-types';
import actionNames from '../action-names';
import {applyStateUpdate} from '../utils/state';

export default (state, action) => {
switch (action.type) {
Expand All @@ -10,7 +11,7 @@ export default (state, action) => {

config.errorPatterns = formatErrorPatterns(config.errorPatterns);

return applyChanges(state, config);
return applyStateUpdate(state, {config});
}

case actionNames.RUN_CUSTOM_GUI_ACTION: {
Expand All @@ -22,7 +23,7 @@ export default (state, action) => {
if (type === CONTROL_TYPE_RADIOBUTTON) {
controls.forEach((control, i) => control.active = (controlIndex === i));

return applyChanges(state, {customGui});
return applyStateUpdate(state, {config: {customGui}});
}

return state;
Expand All @@ -36,13 +37,3 @@ export default (state, action) => {
function formatErrorPatterns(errorPatterns) {
return errorPatterns.map((patternInfo) => ({...patternInfo, regexp: new RegExp(patternInfo.pattern)}));
}

function applyChanges(state, config) {
return {
...state,
config: {
...state.config,
...config
}
};
}
16 changes: 11 additions & 5 deletions lib/static/modules/reducers/grouped-tests/by/meta.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import {handleActiveResults, addGroupItem, sortGroupValues} from '../helpers';
import {ensureDiffProperty} from '../../../utils/state';

export function groupMeta({group, groupKey, ...restArgs}) {
export function groupMeta({group, groupKey, diff = group, ...restArgs}) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Здесь и далее diff в функциях - место, куда нужно записывать изменения. По иерархии такие diff равны тому, чему устанавливаются по умолчанию (в данном случае, group)

Copy link
Member

Choose a reason for hiding this comment

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

Довольно непонятное API. Если ничего не передал в diff значит изменения будут происходить на group.

При этом этот diff нужно в кучу утилит передавать и думать о том как его правильно сформировать и т.д. Выглядит максимально неудобно.

Не пробовал смотреть на immutableJS? Судя по performance графикам от immer - https://immerjs.github.io/immer/performance/ он довольно шустрый.

Copy link
Member Author

Choose a reason for hiding this comment

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

Если ничего не передал в diff значит изменения будут происходить на group

Да, так и задумывалось. Это опциональный аргумент "куда записывать изменения". По умолчанию, если не передать его, запишет в group, плюс, это напоминание, что туда нужно передавать

нужно думать о том, как его правильно сформировать

Да в принципе так же, как и обычное состояние. Просто изменения пишешь туда, и при передаче удостовериваешься, что передаваемый ключ существует с помощью ensureDiffProperty.

Не пробовал смотреть на ImmutableJS?

Пробовал, еще более неудобный

const metaKeys = new Set();
if (groupKey) {
group.byKey[groupKey] = {};
ensureDiffProperty(diff, ['byKey', groupKey]);
}

const resultCb = (result) => {
Expand All @@ -18,14 +19,19 @@ export function groupMeta({group, groupKey, ...restArgs}) {
continue;
}

addGroupItem({group: group.byKey[groupKey], result, value});
addGroupItem({
group: group.byKey[groupKey],
result,
value,
diff: diff.byKey[groupKey]
Comment on lines +23 to +26
Copy link
Member Author

Choose a reason for hiding this comment

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

Так что дальше в функции они прокидываются так же, как и сами их read-only сущности из state

});
}
};

handleActiveResults({...restArgs, resultCb});

group.allKeys = [...metaKeys].sort();
diff.allKeys = [...metaKeys].sort();
if (groupKey) {
group.byKey[groupKey] = sortGroupValues(group.byKey[groupKey]);
diff.byKey[groupKey] = sortGroupValues(diff.byKey[groupKey]);
}
}
15 changes: 11 additions & 4 deletions lib/static/modules/reducers/grouped-tests/by/result.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {get} from 'lodash';
import {handleActiveResults, addGroupItem, sortGroupValues} from '../helpers';
import {isAssertViewError} from '../../../utils';
import {ensureDiffProperty} from '../../../utils/state';
import {ERROR_KEY, RESULT_KEYS} from '../../../../../constants/group-tests';

const imageComparisonErrorMessage = 'image comparison failed';
Expand All @@ -13,21 +14,27 @@ export function groupResult(args) {
throw new Error(`Group key must be one of ${RESULT_KEYS.join(', ')}, but got ${args.groupKey}`);
}

function groupErrors({tree, group, groupKey, errorPatterns = [], ...viewArgs}) {
group.byKey[groupKey] = {};
function groupErrors({tree, group, groupKey, errorPatterns = [], diff = group, ...viewArgs}) {
ensureDiffProperty(diff, ['byKey', groupKey]);

const resultCb = (result) => {
const images = result.imageIds.map((imageId) => tree.images.byId[imageId]);
const errors = extractErrors(result, images);

for (const errorText of errors) {
addGroupItem({group: group.byKey[groupKey], result, value: errorText, patterns: errorPatterns});
addGroupItem({
group: group.byKey[groupKey],
result,
value: errorText,
patterns: errorPatterns,
diff: diff.byKey[groupKey]
});
}
};

handleActiveResults({tree, ...viewArgs, resultCb});

group.byKey[groupKey] = sortGroupValues(group.byKey[groupKey]);
diff.byKey[groupKey] = sortGroupValues(diff.byKey[groupKey]);
}

function extractErrors(result, images) {
Expand Down
8 changes: 4 additions & 4 deletions lib/static/modules/reducers/grouped-tests/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ export function handleActiveResults({tree = {}, viewMode = ViewMode.ALL, filtere
}
}

export function addGroupItem({group, result, value, patterns = []}) {
export function addGroupItem({group, result, value, patterns = [], diff = group}) {
const stringifiedValue = stringify(value);
const {pattern, name} = matchGroupWithPatterns(stringifiedValue, patterns);

// eslint-disable-next-line no-prototype-builtins
if (!group.hasOwnProperty(name)) {
group[name] = {
if ((!group || !group.hasOwnProperty(name)) && !diff.hasOwnProperty(name)) {
diff[name] = {
pattern,
name,
browserIds: [result.parentId],
Expand All @@ -44,7 +44,7 @@ export function addGroupItem({group, result, value, patterns = []}) {
return;
}

const groupItem = group[name];
const groupItem = diff[name];

if (!groupItem.browserIds.includes(result.parentId)) {
groupItem.browserIds.push(result.parentId);
Expand Down
37 changes: 29 additions & 8 deletions lib/static/modules/reducers/grouped-tests/index.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import {produce} from 'immer';
import actionNames from '../../action-names';
import {groupMeta} from './by/meta';
import {groupResult} from './by/result';
import {SECTIONS} from '../../../../constants/group-tests';
import {parseKeyToGroupTestsBy} from '../../utils';
import {applyStateUpdate, ensureDiffProperty} from '../../utils/state';

export default produce((state, action) => {
export default (state, action) => {
switch (action.type) {
case actionNames.INIT_GUI_REPORT:
case actionNames.INIT_STATIC_REPORT:
Expand All @@ -23,30 +23,51 @@ export default produce((state, action) => {
view: {keyToGroupTestsBy, viewMode, filteredBrowsers, testNameFilter, strictMatchFilter}
} = state;
const viewArgs = {viewMode, filteredBrowsers, testNameFilter, strictMatchFilter};
const diff = {groupedTests: {meta: {}}};

if (!keyToGroupTestsBy) {
groupMeta({tree, group: groupedTests.meta, ...viewArgs});
groupMeta({tree, group: groupedTests.meta, diff: diff.groupedTests.meta, ...viewArgs});

return;
return applyStateUpdate(state, diff);
}

const [groupSection, groupKey] = parseKeyToGroupTestsBy(keyToGroupTestsBy);
ensureDiffProperty(diff, ['groupedTests', groupSection]);

const group = groupedTests[groupSection];
const groupDiff = diff.groupedTests[groupSection];

if (groupSection === SECTIONS.RESULT) {
const {config: {errorPatterns}} = state;

return groupResult({tree, group, groupKey, errorPatterns, ...viewArgs});
groupResult({
tree,
group,
groupKey,
errorPatterns,
diff: groupDiff,
...viewArgs
});

return applyStateUpdate(state, diff);
}

if (groupSection === SECTIONS.META) {
return groupMeta({tree, group, groupKey, ...viewArgs});
groupMeta({
tree,
group,
groupKey,
diff: groupDiff,
...viewArgs
});

return applyStateUpdate(state, diff);
}

break;
return state;
}

default:
return state;
}
});
};
17 changes: 10 additions & 7 deletions lib/static/modules/reducers/tree/helpers.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import {get, set} from 'lodash';
import {EXPAND_ALL, EXPAND_ERRORS, EXPAND_RETRIES} from '../../../../constants/expand-modes';
import {getUpdatedProperty} from '../../utils/state';

export function changeNodeState(nodesStateById, nodeId, state) {
export function changeNodeState(nodesStateById, nodeId, state, diff = nodesStateById) {
Object.keys(state).forEach((stateName) => {
const prevStateValue = get(nodesStateById[nodeId], stateName);

if (prevStateValue !== state[stateName]) {
set(nodesStateById, [nodeId, stateName], state[stateName]);
set(diff, [nodeId, stateName], state[stateName]);
}
});
}
Expand All @@ -27,17 +28,19 @@ export function shouldNodeBeOpened(expand, {errorsCb, retriesCb}) {
return false;
}

export function getShownCheckedChildCount(tree, suiteId) {
export function getShownCheckedChildCount(tree, suiteId, diff = tree) {
const {suiteIds = [], browserIds = []} = tree.suites.byId[suiteId];
const checkedChildBrowserCount = browserIds.reduce((sum, browserChildId) => {
const browserState = tree.browsers.stateById[browserChildId];
const shouldBeShown = getUpdatedProperty(tree, diff, ['browsers', 'stateById', browserChildId, 'shouldBeShown']);
const checkStatus = getUpdatedProperty(tree, diff, ['browsers', 'stateById', browserChildId, 'checkStatus']);

return sum + (browserState.shouldBeShown && browserState.checkStatus);
return sum + (shouldBeShown && checkStatus);
}, 0);
const checkedChildSuitesCount = suiteIds.reduce((sum, suiteChildId) => {
const suiteState = tree.suites.stateById[suiteChildId];
const shouldBeShown = getUpdatedProperty(tree, diff, ['suites', 'stateById', suiteChildId, 'shouldBeShown']);
const checkStatus = getUpdatedProperty(tree, diff, ['suites', 'stateById', suiteChildId, 'checkStatus']);

return sum + (suiteState.shouldBeShown && suiteState.checkStatus);
return sum + (shouldBeShown && checkStatus);
}, 0);

return checkedChildBrowserCount + checkedChildSuitesCount;
Expand Down
Loading
Loading