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

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

Merged
merged 1 commit into from
Mar 29, 2017
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## 3.0.0

* Don't save image when option "light" is enabled
* Improved comparing diffs

* major: Do not support node lower than 4.0

## 2.0.0

* major: Add supporting of imagePath instead of image field in fail data
Expand Down
40 changes: 20 additions & 20 deletions lib/errors/base-error.js
Original file line number Diff line number Diff line change
@@ -1,32 +1,32 @@
'use strict';

var _ = require('lodash'),
URI = require('urijs');
const _ = require('lodash');
const URI = require('urijs');

function BaseError(data, config) {
var browserConfig = config.forBrowser(data.browserId);
this._browserCapabilities = browserConfig.desiredCapabilities;
this._quota = new URI(browserConfig.gridUrl).username();
this._timestamp = new Date();
module.exports = class BaseError {
constructor(data, config) {
const browserConfig = config.forBrowser(data.browserId);
this._browserCapabilities = browserConfig.desiredCapabilities;
this._quota = new URI(browserConfig.gridUrl).username();
this._timestamp = new Date();

this.message = data.message || 'Image diff found'; // diff Error don't have "message" key;
this.browserId = data.browserId;
this.sessionId = data.sessionId;
this.name = _.compact([
data.suite.fullName,
data.state && data.state.name,
data.browserId
]).join('.');
}
this.message = data.message || 'Image diff found'; // diff Error don't have "message" key;
this.browserId = data.browserId;
this.sessionId = data.sessionId;
this.name = _.compact([
data.suite.fullName,
data.state && data.state.name,
data.browserId
]).join('.');
Copy link
Member

Choose a reason for hiding this comment

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

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

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

get name() {
  return this._name;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

BaseError.prototype.getData = function() {
return {
getData() {
return {
timestamp: this._timestamp,
message: this.message,
sessionId: this.sessionId,
browserCapabilities: this._browserCapabilities,
seleniumQuota: this._quota
};
}
};

module.exports = BaseError;
32 changes: 6 additions & 26 deletions lib/errors/error-factory.js
Original file line number Diff line number Diff line change
@@ -1,30 +1,10 @@
'use strict';

var q = require('q'),
tempFS = require('../temp-fs'),
const BaseError = require('./base-error');
const ImageError = require('./image-error');

BaseError = require('./base-error'),
ImageError = require('./image-error'),
imageProcessor = require('../image-processor');

exports.buildError = function (data, geminiConfig, options) {
if (!data.imagePath && !data.saveDiffTo) {
return q(new BaseError(data, geminiConfig));
}

var imagePath = data.imagePath || tempFS.resolveImagePath(),
saveImg = data.imagePath ? q : data.saveDiffTo.bind(data);

return saveImg(imagePath)
.then(function() {
if (!options.light) {
return imageProcessor.pngToBase64(imagePath);
}
})
.then(function(base64) {
return new ImageError(data, geminiConfig, {
path: imagePath,
base64: base64
});
});
exports.buildError = (data, config, options) => {
return (data.imagePath || data.saveDiffTo)
? new ImageError(data, config, options)
: new BaseError(data, config);
};
54 changes: 35 additions & 19 deletions lib/errors/image-error.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,39 @@
'use strict';

var _ = require('lodash'),
inherit = require('inherit'),

BaseError = require('./base-error');

var ImageError = inherit(BaseError, {
__constructor: function(data, config, img) {
this.__base(data, config);
this.imagePath = img.path;
this.base64 = img.base64;
this.isDiff = data.equal !== undefined;
},

getData: function() {
return _.defaults(this.__base(), {
base64: this.base64
});
const _ = require('lodash');
const q = require('q');
const fs = require('q-io/fs');

const tempFS = require('../temp-fs');
const BaseError = require('./base-error');
const imageProcessor = require('../image-processor');

module.exports = class ImageError extends BaseError {
constructor(data, config, options) {
super(data, config);

this.imagePath = data.imagePath || tempFS.resolveImagePath();
this.saveImg = data.imagePath ? q : data.saveDiffTo.bind(data);
this.light = options.light;
this.isDiff = !_.isUndefined(data.equal);
}
});

module.exports = ImageError;
getData() {
const baseErrData = super.getData();

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

save() {
return this.saveImg(this.imagePath)
.catch((error) => console.error(`Error occurred while saving image: ${error.stack || error}`));
}

_addBase64(baseErrData) {
return fs.exists(this.imagePath)
.then((isImgExists) => isImgExists || this.save())
.then(() => imageProcessor.pngToBase64(this.imagePath))
.then((base64) => _.extend(baseErrData, {base64}))
.catch((error) => console.error(error.stack || error));
}
};
98 changes: 49 additions & 49 deletions lib/fail-collector.js
Original file line number Diff line number Diff line change
@@ -1,83 +1,83 @@
'use strict';

var _ = require('lodash'),
inherit = require('inherit'),
fs = require('q-io/fs'),
q = require('q'),
const _ = require('lodash');
const fs = require('q-io/fs');
const q = require('q');

errorFactory = require('./errors/error-factory'),
imageProcessor = require('./image-processor');
const errorFactory = require('./errors/error-factory');
const imageProcessor = require('./image-processor');

module.exports = inherit({
__constructor: function (config, opts) {
module.exports = class FailCollector {
constructor(config, opts) {
this._config = config;
this._options = opts || {};
this.fails = [];
},
}

addFail: function (fail) {
this.fails.push(errorFactory.buildError(fail, this._config, this._options));
},
addFail(fail) {
this.fails.push(q.resolve(errorFactory.buildError(fail, this._config, this._options)));
}

collect: function() {
collect() {
return q.all(this.fails)
.then(function(fails) {
return _.groupBy(fails, 'name');
})
.then(this._filterDiffs.bind(this))
.then(this._getFailsData.bind(this))
.then(this._saveToFile.bind(this))
.catch(function (error) {
console.error('Some error while collecting fails: ', error.stack);
});
},
.then((fails) => _.groupBy(fails, 'name'))
.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}`));
}

_filterDiffs: function(fails) {
_filterDiffs(fails) {
return this._getRealFailedTests(fails)
.then(function(realFails) {
return _.omit(fails, realFails);
});
},
.then((realFails) => _.omit(fails, realFails));
}

_getRealFailedTests: function(errors) {
_getRealFailedTests(errors) {
return _(errors)
.map(function(fails) {
.map((fails) => {
return fails.length === this._maxRuns(fails[0].browserId)
&& _.every(fails, {isDiff: true})
&& getRealFailedTestName_(fails);
}.bind(this))
})
.thru(q.all).value()
.then(_.compact);

function getRealFailedTestName_(fails) {
return _(fails)
// remove the comparison of the first diff with yourself
.slice(1)
.map(compareWith_(fails[0]))
.thru(q.all).value()
.then(function(compares) {
return _.every(compares) && fails[0].name;
});
.then((compares) => _.every(compares) && fails[0].name);
}

function compareWith_(reference) {
return function(fail) {
return imageProcessor.compare(reference.imagePath, fail.imagePath);
};
const promise = reference.save();

return (fail) => promise
.then(fail.save())
.then(() => imageProcessor.compare(reference.imagePath, fail.imagePath));
}
},
}

_maxRuns: function(browserId) {
_maxRuns(browserId) {
return this._config.forBrowser(browserId).retry + 1;
},
}

_getFailsData: function (fails) {
return _.mapValues(fails, function (failList) {
return failList.map(function (fail) {
return fail.getData();
});
});
},
_getFailsData(fails) {
const keys = _.keys(fails);

return _(fails)
.map((failList) => _(failList)
.map((fail) => fail.getData())
.thru(q.all)
.value())
.thru(q.all)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.value()
.then((res) => _.zipObject(keys, res));
}
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@up73k up73k Mar 23, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@up73k up73k Mar 24, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@up73k up73k Mar 29, 2017

Choose a reason for hiding this comment

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

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


_saveToFile: function (errors) {
_saveToFile(errors) {
return fs.write('faildump.json', JSON.stringify(errors));
}
});
};
19 changes: 7 additions & 12 deletions lib/plugin.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,15 @@
'use strict';

var FailCollector = require('./fail-collector');
const FailCollector = require('./fail-collector');

module.exports = function (gemini, opts) {
module.exports = (gemini, opts) => {
const fails = new FailCollector(gemini.config, opts);

gemini.on('startRunner', function(runner) {
var fails = new FailCollector(gemini.config, opts);
gemini.on(gemini.events.ERROR, (data) => fails.addFail(data));

runner.on('err', fails.addFail.bind(fails));
gemini.on(gemini.events.RETRY, (data) => fails.addFail(data));

runner.on('retry', fails.addFail.bind(fails));
gemini.on(gemini.events.TEST_RESULT, (data) => data.equal || fails.addFail(data));

runner.on('endTest', function (data) {
data.equal || fails.addFail(data);
});

runner.on('end', fails.collect.bind(fails));
});
gemini.on(gemini.events.END_RUNNER, () => fails.collect());
};
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,8 @@
"unit-test": "istanbul test _mocha -- --recursive test/lib"
},
"dependencies": {
"inherit": "~2.2.1",
"lodash": "^3.10.1",
"looks-same": "2.2.2",
"looks-same": "^3.2.0",
"q": "^1.1.2",
"q-io": "^1.13.2",
"temp": "^0.8.3",
Expand Down
Loading