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

Support configuration of 'sets' #9

Merged
merged 4 commits into from
Mar 30, 2016
Merged

Support configuration of 'sets' #9

merged 4 commits into from
Mar 30, 2016

Conversation

eGavr
Copy link
Contributor

@eGavr eGavr commented Mar 21, 2016

/cc @j0tunn @sipayRT @SwinX

Описание

Теперь в specs появилась возможность задавать в каких браузерах и какие тесты необходимо запускать.

Было:

module.exports = {
    browsers: {
        bro1: {},
        bro2: {},
        bro3: {}
    },

    specs: [
        'common',
        'touch-pad',
        'touch-phone'
    ]
};

Стало:

module.exports = {
    browsers: {
        bro1: {},
        bro2: {},
        bro3: {}
    },    

    specs: [
        {files: ['common'], browsers: ['bro1', 'bro2', 'bro3']}, // или {files: ['common']}, или 'common'
        {files: ['touch-pad'], browsers: ['bro2']},
        {files: ['touch-phone'], browsers: ['bro3']}
    ]
};

Разработка

  • Вынес логику про получение тестов с файловой системы и их обработку в отдельных модуль tests-reader, который возвращает следующий формат данных:

    {chrome: ['/test1.js', '/test2.js'], opera: ['/test1.js', '/test2.js']}
  • Изменил API модуля runner

    Было:

    Runner.prototype.run(['/test1.js', '/test2.js'], ['chrome', 'opera'])

    Стало:

    Runner.prototype.run({chrome: ['/test1.js', 'test2.js'], opera: ['/test1.js', '/test2.js']})

    Второй вариант позволяет конфигурировать запуск определенных тестовых файлов в определенных браузерах.

});

function assignTestFilesToBrowsers(testFiles, sets, browsers) {
warns.warnAboutUnknownBrowsers(getFromSets('browsers', sets), browsers);
Copy link
Member

Choose a reason for hiding this comment

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

а мы не должны тут упасть?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Это обсуждаемо, но я бы не падал (точно также, когда пользователь переопределяет браузеры через командную строку).

@sipayRT
Copy link
Member

sipayRT commented Mar 23, 2016

еще нужен тест про пересечение тестов. Типа, в первом сете указаны тесты ['test1', 'test2'], а во втором - ['test2', 'test3']

expandSets(sets)
])
.spread(function(testFiles, sets) {
return assignTestFilesToBrowsers(testFiles, sets, browsers);
Copy link
Contributor

Choose a reason for hiding this comment

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

_.partialRight

@eGavr
Copy link
Contributor Author

eGavr commented Mar 23, 2016

Нашел багу, пока перевожу задачу в работу.

_ = require('lodash'),
q = require('q');

exports.read = function(testPaths, browsers, sets) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Из имени файла подразумевается, что тут объявлен класс tests-reader, а на деле - утилитная функция.
Почему бы не создать класс и кормить ему в конструкторе конфиг или сеты.
Кажется что в него можно вынести также и это https://github.com/gemini-testing/hermione/pull/9/files#diff-e2ddfd95fbfadc53bebd5b9163b49840R28

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Не совсем понимаю, почему из имени файла подразумевается, что тут класс и зачем здесь нужен класс? Может суть проблемы в том, что файл нужно переименовать ? :)

@eGavr eGavr force-pushed the config/sets branch 2 times, most recently from f2aceac to a180539 Compare March 23, 2016 18:20
@eGavr
Copy link
Contributor Author

eGavr commented Mar 23, 2016

Поправил.

🆙

@eGavr eGavr force-pushed the config/sets branch 3 times, most recently from 6bef5da to 5273bb7 Compare March 24, 2016 09:36
return _this._runInBrowser(browserId, suites, filterFn);
return _(tests)
.map(function(files, browserId) {
return _this._runInBrowser(browserId, files, filterFn);
Copy link
Contributor

Choose a reason for hiding this comment

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

ты бы тогда в объявлении _runInBrowser поправил название аргумента тоже

@eGavr
Copy link
Contributor Author

eGavr commented Mar 25, 2016

@sipayRT @SwinX @j0tunn

🆙

}

function filterSpecs(specs, testFiles, browsers) {
validateUnknownBrowsers(browsers, getBrowsersFromSpecs(specs));
Copy link
Member

Choose a reason for hiding this comment

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

зачем нам тут еще раз проверять наличие неправильных браузеров?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

В первом случае мы проверяем наличие неправильных браузеров, которые указаны непосредственно в опции specs, например, если конфиг такой:

module.exports = {
    browsers: {
        'awesome-browser': {/* awesome-config */}
    },
    specs: [{files: ['hermione/awesome-test'], browsers: ['unknown-browser']}]
};

надо логировать то, что unknown-browser – это какой-то левый браузер.

А во втором случае мы проверяем, неизвестные браузеры, которые переопределены через CLI, например, если с конфигом

module.exports = {
    browsers: {
        'awesome-browser': {/* awesome-config */}
    },
    specs: [{files: ['hermione/awesome-test'], browsers: ['awesome-browser']}]
};

мы вызываем Гермиону

$ hermione -b unknown-browser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

И таки придумал как можно валидировать браузеры одним махом за раз ;)

@eGavr eGavr force-pushed the config/sets branch 7 times, most recently from ddee725 to 2eb2e88 Compare March 28, 2016 14:46
@eGavr eGavr force-pushed the config/sets branch 2 times, most recently from 4efbba7 to be02573 Compare March 28, 2016 14:48
@eGavr
Copy link
Contributor Author

eGavr commented Mar 29, 2016

🆙

{ // run tests associated with this path in all browsers
files: 'tests/desktop' // which are configured in option `browsers`;
}, // the alias for this case is a string 'tests/desktop'

Copy link
Contributor

Choose a reason for hiding this comment

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

можешь прям сюда дописать вариант со строкой tests/deskpad

@j0tunn
Copy link
Contributor

j0tunn commented Mar 29, 2016

еще нужно бы добавить тест, где спеки задаются сразу и строкой и объектом

@j0tunn
Copy link
Contributor

j0tunn commented Mar 29, 2016

и еще тест на такой кейс:

[
    {files: ['/dir'], browsers: 'b1'},
    {files: ['/dir/sub-dir'], browsers: 'b2'}
]

@eGavr
Copy link
Contributor Author

eGavr commented Mar 29, 2016

🆙 please!

@j0tunn
Copy link
Contributor

j0tunn commented Mar 30, 2016

🆗

@eGavr eGavr force-pushed the config/sets branch 2 times, most recently from 4a4a486 to 842bde5 Compare March 30, 2016 10:30
@eGavr eGavr merged commit 079a8b6 into master Mar 30, 2016
@eGavr eGavr deleted the config/sets branch March 30, 2016 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants