fix: do not save diff when option light is true #10

up73k · 2017-02-20T16:46:33Z

No description provided.

j0tunn · 2017-02-21T10:26:05Z

Так нельзя: инстанс ImageError хранит поля isDiff и imagePath, которые в дальнейшем используются для отсеивания реальных диффов (когда все ретраи с одинаковым diff'ом). Если мы это выпилим, то в отчете будут лишние ошибки. По хорошему что нужно сделать:

не сохранять изображение перед созданием инстанса ImageError, а сохранить все данные для того, чтобы это изображение можно было сохранить позже по запросу
при отсеивании реальных диффов - сохранять изображения, и сравнивать между собой
поправить плагин, чтобы collect выполнялся не на END, которое синхронное, а на RUNNER_END, которое асинхронное

И да, про тесты не забудь.

up73k · 2017-03-07T13:06:35Z

на ES6 переведу отдельным коммитом после ревью основных изменений.

DudaGod

Давай сразу переведем отдельным коммитом на es6.

DudaGod · 2017-03-10T17:59:51Z

lib/plugin.js

@@ -15,6 +15,6 @@ module.exports = function (gemini, opts) {
            data.equal || fails.addFail(data);
        });

-        runner.on('end', fails.collect.bind(fails));
+        runner.on('endRunner', fails.collect.bind(fails));
    });
 };


А мы не хотим в рамках данного PR-а переписать тут на использование нового API Gemini? Уменьшится вложенность на один и не нужно будет хардкодить названия эвентов.
Я бы так же добавил опцию enabled как это сделано во всех остальных плагинах, чтобы было консистентно.

Еще у меня предложение обновить версию looksSame. В данном плагине версия - 2.2.2, а текущая версия - 3.2.0.
Так как за это время во первых мы научились работать с анти-алиасингом, а во вторых правильно обрабатывать каретку.

Сюда бы еще eslint наш вкрутить, а то тут ошибок дофига с кодстайлом.
И конфигпарсер как в остальных плагинах =)

на использование нового API Gemini

Переписал.

DudaGod · 2017-03-10T19:44:14Z

lib/errors/image-error.js

-        return _.defaults(this.__base(), {
-                base64: this.base64
-            });
+        const super_ = this.__base.bind(this);


не понял, зачем тебе тут так биндить?

иначе не работает модуль inherit

я уточню почему не работает: в модуле inherit __base переопределяется непосредственно перед вызовом каждого метода (и по моему затирается после вызова). Соответственно в следующем тике в this.__base уже нет ссылки на родительский метод.
Но все это фигня, т.к. при переписываении на es6 inherit можно выбросить

dfilatov/inherit#17

inherit выгасил

DudaGod · 2017-03-10T19:49:34Z

lib/errors/image-error.js

+        }
+
+        return imageProcessor.pngToBase64(this.imagePath)
+            .then((base64) => _.extend(super_(), {base64}));


почему бы последние 2 строки не унести в приватный метод?
Тогда у тебя получится что-то вроде этого:

return this.light ? this.__base() : this._addBase64();

над названием метода нужно подумать )

DudaGod · 2017-03-10T19:52:24Z

lib/errors/error-factory.js

+    return q(new ImageError(data, geminiConfig, {
+        path: imagePath,
+        saveImg: saveImg,
+        light: options.light


я бы пробрасывал весь объект опций, отдельным параметром. Но тут получается у тебя уже аж три параметра... и четвертый добавлять совсем не хочется. Нужно подумать.

DudaGod · 2017-03-10T19:57:35Z

lib/fail-collector.js

@@ -59,8 +59,11 @@ module.exports = inherit({
        }

        function compareWith_(reference) {
+            const promise = reference.save();


давай пустой строкой отобьем

DudaGod · 2017-03-10T20:28:50Z

test/lib/errors/image-error.js

+                });
+        });
+
+        it('should not convert image to base64 if "light" option exist', function() {


DudaGod · 2017-03-10T20:29:49Z

test/lib/errors/image-error.js


-            var failedTest = new ImageError(failedTestError, config, {base64: 'base64-value'}),
-                errorData = failedTest.getData();
+        it('should generate temporary path for image', function() {


этот тест должен быть в другом файле.

DudaGod · 2017-03-10T20:32:56Z

test/lib/errors/image-error.js

+                .then(function() {
+                    assert.notCalled(imageProcessor.pngToBase64);
+                });
+        });


нужен еще тест на проверку того, что вернет метод getData если опци] light установлена

DudaGod · 2017-03-10T20:40:26Z

test/lib/fail-collector.js

+
+            return failCollector.collect()
+                .then(function() {
+                    assert.called(ImageError.prototype.save);


ты тут должен проверить, что save позвался два раза, т.е. для всех фейлов.
И тогда нужен еще один тест, который проверяет, что imageProcessor.compare позвался и с каким аргументами.

DudaGod · 2017-03-10T20:41:27Z

test/lib/fail-collector.js

@@ -63,6 +64,20 @@ describe('fail-collector', function() {
            failCollector = new FailCollector(config);
        });

+        it('should save screenshot of the test failed with diff error type', function() {


should save screenshots for failed tests

DudaGod · 2017-03-21T15:45:01Z

lib/plugin.js

+    gemini.on(gemini.events.ERROR, (data) => fails.addFail(data));
+    gemini.on(gemini.events.RETRY, (data) => fails.addFail(data));
+    gemini.on(gemini.events.TEST_RESULT, (data) => data.equal || fails.addFail(data));
+    gemini.on(gemini.events.END_RUNNER, fails.collect.bind(fails));


зачем тебе тут bind?

Просто так сделай:

gemini.on(gemini.events.END_RUNNER, () => fails.collect());

и давай еще все подписки отобьем друг от друга пустой строкой - тогда будет проще читаться.

DudaGod · 2017-03-21T15:52:37Z

lib/fail-collector.js

+            .then((fails) => this._filterDiffs(fails))
+            .then((fails) => this._getFailsData(fails))
+            .then((fails) => this._saveToFile(fails))
+            .catch((error) => console.error('Some error while collecting fails: ', error.stack));


тут можно template string юзнуть

И что-то мне не очень нравится сообщение - Some error...

DudaGod · 2017-03-21T15:57:00Z

lib/fail-collector.js

            .thru(q.all).value()
            .then(_.compact);

        function getRealFailedTestName_(fails) {
            return _(fails)
+                .slice(1)


для чего тут этот slice?

сравниваем диффы между собой, и убираем сравнение первого дифа с самим собой.

здесь можно оставить коммент

DudaGod · 2017-03-21T16:19:05Z

lib/fail-collector.js

+            .thru(q.all)
+            .value()
+            .then((res) => _.zipObject(keys, res));
+    }


не понял.
Разве так работать не будет? :

return _(fails) .mapValues((failList) => { return _(failList) .map((fail) => fail.getData()) .thru(q.all) .value(); });

неа (я проверил). я над этим местом 3 дня сидел )

Я к тому, что можно без zipObject-а обойтись:

return _(fails) .mapValues((failList) => { return _(failList) .map((fail) => fail.getData()) .thru(q.all) .value(); }) .thru(q.all) .value();

я не уверен, что будет работать. q.all ожидает на вход массив промисов, а ты скармливаешь ему объект, значениями которого являются промисы. В принципе может прокатить, но на выходе скорее всего получится массив. Надо проверять

zipObject нужен чтобы не терялся тайтл теста (входящий аргумент fails - это объект- коллекция массивов с ключами тайтлами тестов).

да, я понимаю для чего он нужен. Мне показалось, что так тоже должно работать и тогда не нужно будет выше сохранять ключи в переменную и в конце вызывать zipObject. Можешь пожалуйста проверить последний мой вариант. Что он возвращает?

твой ваиант возвращает: { 'pager/first-page/1.plain.firefox': { state: 'pending' } }
то есть промисы в стэйте pending еще и не разрезолвлены. Скорее всего, второй q.all не отрабатывает.

DudaGod · 2017-03-21T16:34:26Z

lib/fail-collector.js

+                .map((fail) => fail.getData())
+                .thru(q.all)
+                .value())
+            .thru(q.all)


зачем тут два раза через q.all прогонять?

первый раз резовим промисы в каждом из массивов, второй раз резолвим конечный объект с ними.

DudaGod · 2017-03-21T16:37:03Z

lib/errors/error-factory.js

    if (!data.imagePath && !data.saveDiffTo) {
-        return q(new BaseError(data, geminiConfig));
+        return q(new BaseError({data, config}));


а зачем ты в один объект тут упаковал?

ниже ответил

DudaGod · 2017-03-21T16:39:29Z

lib/errors/base-error.js

-}
+        this.message = opts.data.message || 'Image diff found'; // diff Error don't have "message" key;
+        this.browserId = opts.data.browserId;
+        this.sessionId = opts.data.sessionId;


теперь доступ к опциям слишком длинный получается - opts.data.some, а не просто opts.some

DudaGod · 2017-03-21T16:41:20Z

lib/errors/error-factory.js

-                base64: base64
-            });
-        });
+    return q(new ImageError({data, config, img: {


кажется не лучшее решение все в один гигантский объект упаковывать.

я бы все таки лучше оставил как было до этого

ты сам писал выше: "я бы пробрасывал весь объект опций, отдельным параметром. Но тут получается у тебя уже аж три параметра... и четвертый добавлять совсем не хочется" , поэтому и исправил.

верну как было

ну ведь data и config это совершенно другие объекты. А я говорил, про объект options в котором у тебя сейчас хранится поле light

DudaGod · 2017-03-21T17:08:31Z

lib/errors/image-error.js

-module.exports = ImageError;
+    _addBase64() {
+        return fs.exists(this.imagePath)
+            .then((isFileExists) => isFileExists ? _.noop() : q.fcall(this.save.bind(this)))


Я бы переменную назвал - isImgExists.
Так же я не понял зачем тебе тут q.fcall?
Ты же здесь можешь так написать:

.then((isImgExists) => isImgExists || this.save()

DudaGod · 2017-03-21T17:10:45Z

lib/errors/image-error.js

+        return fs.exists(this.imagePath)
+            .then((isFileExists) => isFileExists ? _.noop() : q.fcall(this.save.bind(this)))
+            .then(() => imageProcessor.pngToBase64(this.imagePath))
+            .then((base64) => _.extend(super.getData(), {base64}))


ты заметил, что ты получается super.getData зовешь в обоих случаях?
И когда this.light - true и когда false.
Может тогда будем вызывать его только в одном место? В this.getData?

решил оставить так, чтобы не ломать промисы.

я считаю, что в этом случае нужно исправить.
Тем более я не пойму как ты их сломаешь, если super.getData() возвращает обычный объект?

_.extend не дожидается выполнения промиса _addBase64

ниче не понял.
В данном случае extend выполнится после того как разрезолвится вызов pngToBase64.
Вместо super.getData() ты должен туда просто подпихнуть переменную которую выше объявишь

ок, прокинул через параметр в _addBase64

DudaGod · 2017-03-22T06:50:08Z

test/lib/errors/base-error.js

@@ -18,29 +18,29 @@ describe('errors/base-error', function() {
            }),
            config = mkConfigStub();


тут две переменные и каждая из них должна объявляться отдельно, а не через запятую

DudaGod · 2017-03-22T07:08:08Z

lib/errors/base-error.js

+            opts.data.suite.fullName,
+            opts.data.state && opts.data.state.name,
+            opts.data.browserId
+        ]).join('.');


а почему у нас часть параметров public, а часть private?

исторически сложилось, решил так оставить

DudaGod · 2017-03-22T07:10:41Z

test/lib/errors/base-error.js

-                var failedTestError = mkErrorStub(),
-                    config = mkConfigStub(),
-                    failedTest = new BaseError(failedTestError, config);
+    describe('getData()', () => {


а нафига нам скобочки в описании? Итак же понятно, что это метод

DudaGod · 2017-03-22T07:11:03Z

test/lib/errors/base-error.js

-                    failedTest = new BaseError(failedTestError, config);
+    describe('getData()', () => {
+        ['timestamp', 'message', 'sessionId', 'browserCapabilities', 'seleniumQuota'].forEach((key) => {
+            it('should return object with ' + key + ' value', () => {


давай тут template string юзнем

DudaGod · 2017-03-22T07:12:26Z

test/lib/errors/base-error.js

-                    config = mkConfigStub(),
-                    failedTest = new BaseError(failedTestError, config);
+    describe('getData()', () => {
+        ['timestamp', 'message', 'sessionId', 'browserCapabilities', 'seleniumQuota'].forEach((key) => {


я бы так писал:

[ 'timestamp', 'message', 'sessionId', 'browserCapabilities', 'seleniumQuota' ].forEach((key) => {...});

DudaGod · 2017-03-22T10:19:23Z

test/lib/utils.js


-var errorDefaults = {
+const errorDefaults = {
        suite: {fullName: 'suite-fullname'},
        state: {name: 'state-name'},
        browserId: 'browserId'
    };


отступы поехали

DudaGod · 2017-03-22T10:20:19Z

test/lib/utils.js


-var errorDefaults = {
+const errorDefaults = {
        suite: {fullName: 'suite-fullname'},
        state: {name: 'state-name'},
        browserId: 'browserId'
    };

 module.exports = {


зачем?
Почему бы просто не експортить каждый метод отдельно?

exports.mkErrorStub = () = {...}

кажется, что разницы особой нет

DudaGod · 2017-03-22T10:21:47Z

test/lib/plugin.js

+
+describe('plugin', () => {
+    const sandbox = sinon.sandbox.create();
+    const geminiEvents = mkGeminiEvents();


почему бы сразу не сделать:

const geminiEvents = utils.mkGeminiEvents();

убрал utils.mkGeminiEvents

DudaGod · 2017-03-22T10:23:24Z

test/lib/utils.js

            equal: false,
            saveDiffTo: sinon.stub().returns(q())
-        }, errorDefaults);
+        },
+        errorDefaults


это не должно на следующую строку переносится

DudaGod · 2017-03-22T10:23:46Z

test/lib/utils.js

+            RETRY: 'retry',
+            ERROR: 'err',
+            TEST_RESULT: 'testResult'
+        };


нужен ли нам для этого хелпер?
Если он по идеи только в plugin.js используется?

DudaGod · 2017-03-27T09:55:08Z

lib/errors/base-error.js

+            data.suite.fullName,
+            data.state && data.state.name,
+            data.browserId
+        ]).join('.');


я бы все таки сделал все поля приватными и через геттер расшарил те которые нужны снаружи.
На сколько я понял снаружи используются только name и browserId.

А sessionId и message вообще снаружи не юзаются.
Т.е. достаточно написать так (для тех которые снаружи юзаются):

get name() { return this._name; }

задача не про это. Я так понял этот файл был затронут просто переводом на es6

DudaGod · 2017-03-27T14:20:23Z

lib/errors/base-error.js

+                sessionId: this.sessionId,
+                browserCapabilities: this._browserCapabilities,
+                seleniumQuota: this._quota
+            };


отступы поехали

DudaGod · 2017-03-27T14:49:22Z

lib/errors/error-factory.js

+            path: imagePath,
+            saveImg,
+            light: options.light
+        }


теперь же логику про инициализацию imagePath и saveImg нужно уносить в ImageError.
Так как error-factory ничего про это знать не должен. Его задача создать инстансы ImageError и BaseError в зависимости от переданных параметров. А что происходит внутри его не должно касаться.
Т.е. здесь тебе просто нужно сделать так:

return new ImageError(data, config, options);

DudaGod · 2017-03-27T14:52:04Z

lib/errors/error-factory.js

    if (!data.imagePath && !data.saveDiffTo) {
-        return q(new BaseError(data, geminiConfig));
+        return q(new BaseError(data, config));


зачем нам тут оборачивать вызов конструктора BaseError в q? Так же само ниже мы оборачиваем конструктор ImageError в q. Здесь правильнее было бы обернуть сам вызов buildError если нужно.
Т.е. вот тут.

DudaGod · 2017-03-27T14:55:48Z

lib/errors/error-factory.js

+            saveImg,
+            light: options.light
+        }
+    ));


в итоге будет выглядеть это так:

exports.buildError = (data, config, options) => { return data.imagePath || data.saveDiffTo ? new ImageError(data, config, options) : new BaseError(data, config); };

DudaGod · 2017-03-27T15:08:54Z

lib/errors/image-error.js

+    }
+
+    getData() {
+        const baseData = super.getData();


я бы эту переменную назвал baseErrData

DudaGod · 2017-03-27T15:09:30Z

lib/errors/image-error.js

-            });
+        return this.light
+            ? baseData
+            : this._addBase64(baseData);


как вариант:

return this._opts.light ? baseErrData : this._addBase64(baseErrData);

DudaGod · 2017-03-27T15:12:01Z

lib/errors/image-error.js

-module.exports = ImageError;
+    save() {
+        return this.saveImg(this.imagePath)
+            .catch((error) => console.error(error.stack || error));


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

.catch((error) => console.error(`Error occurred while saving image: $(error)`));

$(error) -> ${error}

чаще всего хочется видеть стэк

DudaGod · 2017-03-27T15:13:17Z

lib/errors/image-error.js

+            .catch((error) => console.error(error.stack || error));
+    }
+
+    _addBase64(data) {


давай тут переименуем параметр который ты передаешь. Я бы вместо data назвал ее baseErrData

DudaGod · 2017-03-27T15:26:30Z

lib/fail-collector.js

+            .thru(q.all)
+            .value()
+            .then((res) => _.zipObject(keys, res));
+    }


да, я понимаю для чего он нужен. Мне показалось, что так тоже должно работать и тогда не нужно будет выше сохранять ключи в переменную и в конце вызывать zipObject. Можешь пожалуйста проверить последний мой вариант. Что он возвращает?

DudaGod · 2017-03-27T15:28:41Z

test/lib/errors/base-error.js

                suite: {
                    fullName: 'suite-fullname'
                },
                state: {
                    name: 'state-name'
                },
                browserId: 'browserId'
-            }),
-            config = mkConfigStub();
+            });


отступы уехали

DudaGod · 2017-03-27T15:36:37Z

test/lib/errors/error-factory.js

-                    assert.calledOnce(imageProcessor.pngToBase64);
-                });
+                .then((errorData) => {
+                    errorData.saveImg();


этот тест уйдет в image-error если сделать как я выше предложил. Плюс станет понятнее.

DudaGod · 2017-03-27T15:42:39Z

test/lib/errors/image-error.js

+        it('should return extended data with "base64" key', () => {
+            imageProcessor.pngToBase64.returns(q.resolve('base64-value'));
+
+            return mkImgErrGetData({light: false})


зачем? Ты тем самым сделал его еще сложнее. Так как из названия теперь не сразу понятно, что оно делает. А если назвать mkImgErr - понятно, что создается инстанс и ты сразу дальше зовешь .getData. Плюс этот хелпер может юзаться в будущем для других методов.

DudaGod · 2017-03-27T16:36:24Z

test/lib/utils.js


 module.exports = {
-    mkErrorStub: function(opts) {
+    mkErrorStub: (opts) => {
        opts = opts || {};
        return _.defaults(opts, errorDefaults);


давай хелперы консистентно друг другу сделаем.
У тебя в двух _.defaults сразу зовется, а в двух в самом конце.

j0tunn · 2017-03-29T07:17:06Z

lib/fail-collector.js

+            .then((fails) => this._filterDiffs(fails))
+            .then((fails) => this._getFailsData(fails))
+            .then((fails) => this._saveToFile(fails))
+            .catch((error) => console.error(`Error occurred while collecting fails: $(error.stack)`));


$(error.stack) -> ${error.stack}. Но вообще я не понимаю зачем здесь менять то что было на template literals

j0tunn · 2017-03-29T07:18:21Z

lib/fail-collector.js

-            }.bind(this))
+            .map(((fails) => fails.length === this._maxRuns(fails[0].browserId)
+                && _.every(fails, {isDiff: true})
+                && getRealFailedTestName_(fails)))


не надо так писать. Это уже далеко не однострочник

DudaGod · 2017-03-29T14:12:27Z

/ok

levonet added the in progress label Feb 20, 2017

up73k force-pushed the light-error branch from 04265cb to 4b6bac8 Compare March 7, 2017 13:04

DudaGod requested changes Mar 10, 2017

View reviewed changes

DudaGod requested changes Mar 21, 2017

View reviewed changes

DudaGod requested changes Mar 22, 2017

View reviewed changes

DudaGod reviewed Mar 27, 2017

View reviewed changes

j0tunn reviewed Mar 29, 2017

View reviewed changes

up73k force-pushed the light-error branch from 2316ac6 to 0cfbc78 Compare March 29, 2017 13:49

DudaGod approved these changes Mar 29, 2017

View reviewed changes

fix: Correct errors saving, Convert to ES6

32ee92b

up73k force-pushed the light-error branch from 0cfbc78 to 32ee92b Compare March 29, 2017 14:13

up73k merged commit bbb1a6d into master Mar 29, 2017

levonet removed the in progress label Mar 29, 2017

up73k deleted the light-error branch March 29, 2017 14:23

		@@ -18,29 +18,29 @@ describe('errors/base-error', function() {
		}),
		config = mkConfigStub();

fix: do not save diff when option light is true #10

fix: do not save diff when option light is true #10

Conversation

up73k commented Feb 20, 2017

j0tunn commented Feb 21, 2017

up73k commented Mar 7, 2017

DudaGod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

up73k Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

up73k Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

up73k Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

up73k Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

up73k Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

up73k Mar 14, 2017 •

edited

Loading

up73k Mar 11, 2017 •

edited

Loading

up73k Mar 23, 2017 •

edited

Loading

up73k Mar 24, 2017 •

edited

Loading

up73k Mar 29, 2017 •

edited

Loading