feat: wrap tests running in command #241

rostik404 · 2018-03-12T14:25:36Z

rostik404 · 2018-03-12T14:26:11Z

package.json

@@ -35,7 +35,7 @@
    "bluebird-q": "^2.1.1",
    "chalk": "^1.1.1",
    "clear-require": "^1.0.1",
-    "commander": "^2.12.2",
+    "commander": "gemini-testing/commander.js",


Сидим на форке, пока не вольется PR tj/commander.js#778

лучше сидеть на какой-то конкретной ветке, а не на мастере. Мало ли - решим еще чего-то поправить

j0tunn · 2018-03-13T14:23:04Z

lib/cli/index.js

+                    logger.error(err.stack || err);
+                    process.exit(1);
+                })
+                .done();


Зачем здесь done?

j0tunn · 2018-03-13T14:25:27Z

package.json

@@ -35,7 +35,7 @@
    "bluebird-q": "^2.1.1",
    "chalk": "^1.1.1",
    "clear-require": "^1.0.1",
-    "commander": "^2.12.2",
+    "commander": "gemini-testing/commander.js",


лучше сидеть на какой-то конкретной ветке, а не на мастере. Мало ли - решим еще чего-то поправить

j0tunn · 2018-03-13T14:28:46Z

test/lib/cli/index.js

    });

    afterEach(() => sandbox.restore());

    it('should show information about config overriding on "--help"', () => {
-        onParse((parser) => parser.emit('--help'));
+        process.argv.push('--help');
+        hermioneCli.run();


hermioneCli.run() возвращает промис. Ты его здесь не дожидаешься

j0tunn · 2018-03-13T14:29:25Z

test/lib/cli/index.js

-        onParse((parser) => parser.emit('--help'));
+        process.argv.push('--help');
+        hermioneCli.run();
+        process.argv.pop();


если на строчке выше тест упадет, то этого не случится, и будет у тебя process.argv модифицированный

Да, тебе нужно это разруливать на уровне хуков в тестах или добавить try {} finally {}

tormozz48 · 2018-03-13T17:57:51Z

lib/cli/index.js

+                    grep: program.grep,
+                    updateRefs: program.updateRefs
+                })
+                .then((success) => process.exit(success ? 0 : 1))


.then((success) => process.exit(Number(!success)))

@tormozz48 ты реально считаешь что так будет понятнее? о.О

tormozz48 · 2018-03-13T18:02:20Z

test/lib/cli/index.js

-        onParse((parser) => parser.emit('--help'));
+        process.argv.push('--help');
+        hermioneCli.run();
+        process.argv.pop();


Да, тебе нужно это разруливать на уровне хуков в тестах или добавить try {} finally {}

tormozz48 · 2018-03-13T18:02:53Z

test/lib/cli/index.js

-        onParse((parser) => parser.config = '.conf.hermione.js');
+        process.argv.push('--config', '.conf.hermione.js');
+        hermioneCli.run();
+        process.argv = process.argv.slice(0, -2);


См. замечание выше

j0tunn · 2018-03-15T05:33:30Z

test/lib/cli/index.js

-            fn(this);
+            parser = fn(this);
+
+            actionFn && actionFn(_.get(parser, 'args'));


const actionFn = Command.prototype.action.lastCall.args[0]; actionFn && actionFn(parser.args);

j0tunn · 2018-03-15T05:34:53Z

test/lib/cli/index.js

+        Command.prototype.action.callsFake((fn) => actionFn = fn);
+    };
+
+    const run_ = async (promise = q(true)) => {


этот дефолтный аргумент будет шариться между всеми вызовами run_

async, но внутри нет await. Забыл await promise;? Вообще надо бы проверить, что это будет работать

Dmitriy-kiselyov · 2018-03-15T13:00:55Z

test/lib/cli/index.js

@@ -12,16 +13,22 @@ const any = sinon.match.any;

 describe('cli', () => {
    const sandbox = sinon.sandbox.create();
+    let parser;


parser можно объявить внутри onParse. В дискрайбе он не используется

Dmitriy-kiselyov · 2018-03-15T13:34:30Z

test/lib/cli/index.js

+        sandbox.stub(Command.prototype, 'action');
+        sandbox.stub(Hermione.prototype, 'run').resolves();
+
+        onParse();


Меня выше fn = _.noop и _.get(parser, 'args') сильно запутали. Может лучше написатьonParse(() => ({})), и ниже по коду везде _.set использовать?

Немного муторно, конечно, но функция выше легче для восприятия станет, как по мне. Но это не критично, я просто предлагаю варианты)

А вообще, onParse() здесь нужен?

Нужен, в каждом тесте нужно застабать парсер, в beforeEach это для тех тестов, которым не нужна специфичная функция парсинга

Он же и так застабан будет. Просто нужно ли специфицировать, что именно будет при парсинге происходить?

Dmitriy-kiselyov · 2018-03-15T13:37:01Z

test/lib/cli/index.js

-    it('should create Hermione instance', () => {
-        return hermioneCli.run()
-            .then(() => assert.calledOnce(Hermione.create));
+    it('should create Hermione instance', async () => {


async тут не используется
и ниже в нескольких местах тоже не используется

Dmitriy-kiselyov · 2018-03-15T14:19:41Z

test/lib/cli/index.js

+    it('should not pass any grep rule if it was not specified from cli', async () => {
+        hermioneCli.run();
+
+        assert.calledWithMatch(Hermione.prototype.run, any, {grep: undefined});


await actionPromise?

Dmitriy-kiselyov · 2018-03-15T14:29:07Z

test/lib/cli/index.js

+    it('should not pass any browsers if they were not specified from cli', async () => {
+        hermioneCli.run();
+
+        assert.calledWithMatch(Hermione.prototype.run, any, {browsers: undefined});


await actionPromise?

Dmitriy-kiselyov · 2018-03-15T14:30:49Z

test/lib/cli/index.js

    });

-    it('should exit with code 1 if tests fail', () => {
+    it('should exit with code 1 if tests fail', async () => {
        Hermione.prototype.run.returns(q(false));


Забыл на resolves поменять или специально?

Dmitriy-kiselyov · 2018-03-15T14:31:52Z

test/lib/cli/index.js

    });

-    it('should log an error on reject if stack does not exist', () => {
+    it('should log an error on reject if stack does not exist', async () => {
        Hermione.prototype.run.returns(q.reject('some-error'));


Забыл на rejects поменять или специально?

Обсудили голосом

DudaGod · 2018-03-15T14:27:05Z

test/lib/cli/index.js

@@ -12,16 +13,22 @@ const any = sinon.match.any;

 describe('cli', () => {
    const sandbox = sinon.sandbox.create();


sinon.sandboxCreate() побыстрее работает )

DudaGod · 2018-03-15T14:30:58Z

test/lib/cli/index.js

@@ -30,137 +37,163 @@ describe('cli', () => {
        sandbox.stub(process, 'exit');

        sandbox.stub(Command.prototype, 'parse');
+        sandbox.stub(Command.prototype, 'action');
+        sandbox.stub(Hermione.prototype, 'run').resolves();


а чего ты перенес этот стаб сюда? Удобно же было, что все методы гермионы стабаются рядышком.

sipayRT · 2018-03-15T13:00:04Z

test/lib/cli/index.js

+
+            if (Command.prototype.action.lastCall) {
+                const actionFn = Command.prototype.action.lastCall.args[0];
+                actionPromise = actionFn(_.get(parser, 'args'));


выше parser = fn(this) || {}; и здесь не нужен будет _.get

sipayRT · 2018-03-15T13:00:50Z

test/lib/cli/index.js

@@ -12,16 +13,22 @@ const any = sinon.match.any;

 describe('cli', () => {
    const sandbox = sinon.sandbox.create();
+    let parser;


переменную нужно ниже объявить, в onParse

rostik404 commented Mar 12, 2018

View reviewed changes

j0tunn requested changes Mar 13, 2018

View reviewed changes

tormozz48 approved these changes Mar 13, 2018

View reviewed changes

rostik404 force-pushed the run-test-command branch 2 times, most recently from 438f773 to 20e21c9 Compare March 14, 2018 22:18

j0tunn requested changes Mar 15, 2018

View reviewed changes

Dmitriy-kiselyov reviewed Mar 15, 2018

View reviewed changes

Dmitriy-kiselyov suggested changes Mar 15, 2018

View reviewed changes

DudaGod reviewed Mar 15, 2018

View reviewed changes

sipayRT approved these changes Mar 15, 2018

View reviewed changes

rostik404 force-pushed the run-test-command branch from f9d8cb4 to b247f10 Compare March 15, 2018 15:44

feat: wrap tests running in command

fcbd8bf

rostik404 force-pushed the run-test-command branch from b247f10 to fcbd8bf Compare March 15, 2018 15:46

rostik404 merged commit 735097a into master Mar 15, 2018

rostik404 deleted the run-test-command branch March 15, 2018 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: wrap tests running in command #241

feat: wrap tests running in command #241

rostik404 commented Mar 12, 2018

rostik404 Mar 12, 2018

j0tunn Mar 13, 2018

j0tunn Mar 13, 2018

j0tunn Mar 13, 2018

j0tunn Mar 13, 2018

j0tunn Mar 13, 2018

tormozz48 Mar 13, 2018

tormozz48 Mar 13, 2018

j0tunn Mar 13, 2018

tormozz48 Mar 13, 2018

tormozz48 Mar 13, 2018

j0tunn Mar 15, 2018

j0tunn Mar 15, 2018

j0tunn Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

rostik404 Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

Dmitriy-kiselyov Mar 15, 2018

rostik404 Mar 15, 2018

DudaGod Mar 15, 2018

DudaGod Mar 15, 2018

sipayRT Mar 15, 2018

sipayRT Mar 15, 2018

		@@ -12,16 +13,22 @@ const any = sinon.match.any;

		describe('cli', () => {
		const sandbox = sinon.sandbox.create();

feat: wrap tests running in command #241

feat: wrap tests running in command #241

Conversation

rostik404 commented Mar 12, 2018

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