feat: add grouping by error type #213

CatWithApple · 2019-03-25T13:57:33Z

Аdded a button "Group by error" that groups tests by error messages. Patterns for grouping can be specified in the configuration.
I will write more unit tests after approving main code.

rostik404 · 2019-03-27T15:06:22Z

README.md

@@ -24,6 +24,10 @@ directory.
 * **baseHost** (optional) - `String` - it changes original host for view in the browser; by default original host does not change
 * **scaleImages** (optional) – `Boolean` – fit images into page width; `false` by default
 * **lazyLoadOffset** (optional) - `Number` - allows you to specify how far above and below the viewport you want to begin loading images. Lazy loading would be disabled if you specify 0. `800` by default.
+* **errorPatterns** (optional) - `Array` - error message patterns for 'Group by error' mode. 
+Array element must be `Object` ({'*name*': `String`, '*pattern*': `String`}) or `String` (interpret as *name* and *pattern*).
+Test will be associated with group if `errorTest.message.match(new RegExp(pattern))`. 


Давай как-то так - Test will be associated with group if test error matches on group error pattern, чтобы в доку куски кода не вставлять.

rostik404 · 2019-03-27T15:13:22Z

lib/static/components/error-group.js

+
+    constructor(props) {
+        super(props);
+        this._toggleState = this._toggleState.bind(this);


А чего не через стрелочную функцию, чтобы без биндинга?
https://medium.com/silesis/handle-events-in-react-with-arrow-functions-ede88184bbb

rostik404 · 2019-03-27T15:19:15Z

lib/static/components/error-group.js

+
+        const body = this.state.collapsed
+            ? null
+            : <div className="error_group__body error_group__body_guided">


error-group__body_guided - го по БЭМ-у

rostik404 · 2019-03-27T15:29:38Z

lib/static/components/error-group.js

+            <div className={className}>
+                <div className="error_group__title" onClick={this._toggleState} title={group.pattern}>
+                    <div className="error_group__name">{group.name}</div>
+                    <div className="error_group__count">&nbsp;({group.count})</div>


Кажется, здесь перебор с дивами. italic можно сделать с помощью тега <i>

diveloper жи есть

rostik404 · 2019-03-27T15:30:31Z

lib/static/components/error-groups-list.js

+export default connect(
+    (state) => ({
+        groupedErrors: state.groupedErrors
+    })


го в 1 строчку

rostik404 · 2019-03-27T16:35:40Z

lib/static/modules/utils.js

+ * @param {string} [filterByName]
+ * @return {array}
+ */
+function groupErrors(suites, errorPatterns, filteredBrowsers, filterByName = '') {


filterByName - звучит, как название функции
Я бы nameFilterPattern назвал

rostik404 · 2019-03-27T16:37:17Z

lib/static/modules/utils.js

+                    group.tests[testName].push(browserName);
+                    group.count++;
+                }
+            }


Давай этот for перепишем через reduce и в функцию вынесем. groupErrors занимает 52 строчки - ее точно рефакторить нужно.

функцию вынес. не понял, чем поможет reduce читаемости

rostik404 · 2019-03-27T16:38:24Z

lib/static/modules/utils.js

+                }
+                if (!group.tests[testName].includes(browserName)) {
+                    group.tests[testName].push(browserName);
+                    group.count++;


Этот каунтер точно нужен?
Нельзя потом взять length от Object.keys(group.tests)?

Нужно брать количество упавших тестов по браузерам, чтобы числа были в соответствии со статистикой сверху

rostik404 · 2019-03-27T16:39:47Z

lib/static/modules/utils.js

+                if (!group.tests.hasOwnProperty(testName)) {
+                    group.tests[testName] = [];
+                }
+                if (!group.tests[testName].includes(browserName)) {


Если group.tests[testName] сделать set-ом, а не массивом, эта проверка уйдет

rostik404 · 2019-03-27T17:22:05Z

lib/static/modules/utils.js

+    }
+    return {
+        pattern: errorText
+    };


в одну строчку

зачем?

+1 за одну строчку, в читаемости код не потеряет, но будет короче. В этом и плюс.

CatWithApple · 2019-03-28T14:55:46Z

fixed.
added tests.

DudaGod

section-common сейчас слишком умный. Он ничего не должен знать про группировку по ошибкам

DudaGod · 2019-03-29T08:26:08Z

README.md

@@ -24,6 +24,10 @@ directory.
 * **baseHost** (optional) - `String` - it changes original host for view in the browser; by default original host does not change
 * **scaleImages** (optional) – `Boolean` – fit images into page width; `false` by default
 * **lazyLoadOffset** (optional) - `Number` - allows you to specify how far above and below the viewport you want to begin loading images. Lazy loading would be disabled if you specify 0. `800` by default.
+* **errorPatterns** (optional) - `Array` - error message patterns for 'Group by error' mode. 
+Array element must be `Object` ({'*name*': `String`, '*pattern*': `String`}) or `String` (interpret as *name* and *pattern*).


Не понял как в данном случае будет выглядеть элемент массива переданный строкой. Чем я должен имя от паттерна отделить? Пробелом?

Я бы еще ниже добавил пример.

если значение строка - то она интерпретируется и как шаблон и как название группы.
никаких разделителей.
пример добавлю

DudaGod · 2019-03-29T08:28:52Z

lib/config.js

+        errorPatterns: option({
+            defaultValue: configDefaults.errorPatterns,
+            parseEnv: JSON.parse,
+            validate: assertArray('errorPatterns')


я бы еще проверил, что элементы массива являются объектами или строками

DudaGod · 2019-03-29T08:31:47Z

lib/static/components/controls/test-name-filter-input.js

@@ -4,30 +4,30 @@ import React, {Component} from 'react';
 import PropTypes from 'prop-types';
 import {bindActionCreators} from 'redux';
 import {connect} from 'react-redux';
-import _ from 'lodash';
+import {debounce} from 'lodash';


DudaGod · 2019-03-29T08:32:45Z

lib/static/components/controls/test-name-filter-input.js

    (dispatch) => ({actions: bindActionCreators(actions, dispatch)})
-)(FilterByNameInput);
+)(TestNameFilterInput);


это можно было бы отдельным коммитом сделать

DudaGod · 2019-03-29T08:34:34Z

lib/static/components/error-group.js

+        this.state = {
+            collapsed: true
+        };
+    }


красивее будет вот так:

state = { collapsed: true }

В этом случае конструктор определять не нужно

DudaGod · 2019-03-29T11:15:08Z

lib/static/modules/utils.js

+        } else if (typeof patternInfo === 'object') {
+            return {
+                pattern: patternInfo.pattern,
+                name: patternInfo.name,


почему у тебя в итоге в зависимости от типа получаются объекты с разными полями? Давай сделаем, чтобы поля у них были одинаковые. Тебе же так в коде удобнее будет.

DudaGod · 2019-03-29T11:43:42Z

lib/static/modules/utils.js

+    errorPatterns = normalizeErrorPatterns(errorPatterns);
+    const testWithErrors = {};
+
+    extractErrors(suites, testWithErrors);


такая конструкция выглядет очень странной.

Почему так не написать:

const errors = extractErrors(suites);

DudaGod · 2019-03-29T11:53:34Z

lib/static/modules/utils.js

+        if (suite.children) {
+            extractErrors(suite.children, testWithErrors);
+        }
+    }


Можно же проще написать.

function extractErrors(suites) { const extract = (node) => { if (node.children) { return flatMap(node.children, extract); } if (node.browsers) { return flatMap(node.browsers, extract); } if (isSuccessStatus(node.result.status)) { return []; } const broResults = browser.retries.concat(browser.result); const errors = extractErrorsFromRetries(broResults); return {[node.name]: errors}; }); return flatMap(suites, extract); }

DudaGod · 2019-03-29T11:56:04Z

lib/static/modules/utils.js

+        }
+        if (retry.error && retry.error.message) {
+            errorsInRetry.add(retry.error.message);
+        }


[retry].concat(retry.imagesInfo).forEach(({error}) => { if (_.get(error, 'message')) { errorsInRetry.add(error.message); } });

DudaGod · 2019-03-29T12:05:35Z

lib/static/modules/utils.js

+    }
+    return {
+        pattern: errorText
+    };


+1 за одну строчку, в читаемости код не потеряет, но будет короче. В этом и плюс.

CatWithApple · 2019-04-01T13:13:45Z

fixed

DudaGod

в остальном /ok

DudaGod · 2019-04-01T15:23:07Z

README.md

+                        name: 'Cannot read property of undefined',
+                        pattern: 'Cannot read property .* of undefined'
+                    }
+                ]


DudaGod · 2019-04-01T15:25:19Z

lib/config.js

+                pattern: patternInfo
+            };
+        }
+        return patternInfo;


Я бы так написал:

return _.isString(pattern) ? {name: pattern, pattern} : pattern;

DudaGod · 2019-04-01T15:28:42Z

lib/config.js

+            }
+        }
+    }
+};


DudaGod · 2019-04-01T15:29:21Z

lib/config.js

+        }),
+        errorPatterns: option({
+            defaultValue: configDefaults.errorPatterns,
+            parseEnv: JSON.parse,


а почему ты тут parseCli не указал?

Ты думаешь, кто-нибудь захочет передать список шаблонов ошибок через cli?

DudaGod · 2019-04-01T15:30:43Z

lib/static/components/controls/test-name-filter-input.js

        actions: PropTypes.object.isRequired
    }

    constructor(props) {
        super(props);

        this.state = {
-            filterByName: this.props.filterByName
+            testNameFilter: this.props.testNameFilter
        };


Чего не захотел от конструктора избавиться? Тебе запись такая не нравиться?

DudaGod · 2019-04-01T16:58:57Z