Skip to content

Conversation

@ret-Phoenix
Copy link
Contributor

gitsync export c:\store\ -minversion 4 -maxversion 7
gitsync export c:\store\ -minversion 3
gitsync export c:\store\ -maxversion 5


Функция СоздатьКомандныйФайл(Знач Путь = "")

Файл = Новый КомандныйФайл();
Copy link
Member

Choose a reason for hiding this comment

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

А давай библиотеку 1commands использовать для этого вот всего?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Можно, свой тест сделал копипастом на основе lib\gitsync\tests\git-sync-test.os

Copy link
Member

Choose a reason for hiding this comment

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

А давай библиотеку 1commands использовать для этого вот всего?

Ага, специально для таких реализаций я и делал данную библиотеку

Copy link
Member

@artbear artbear left a comment

Choose a reason for hiding this comment

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

Зачем нужен еще один тестовый файл хранилища 1С?
у нас уже есть в проекте в папке tests/fixtures набор бинарных файлов, необходимых для тестирования.

Предлагаю использовать в тестах уже существующее хранилище.

@ret-Phoenix
Copy link
Contributor Author

ret-Phoenix commented Jan 20, 2017

Имеющееся хранилище не позволяет достоверно проверить работу, т.к. там всего 3 коммита.
Если добавлять еще коммиты - надо переделывать основной тест gitsync, на что я не решился.


КонецФункции

Функция ПроверитьСуществованиеФайлаКаталога(парамПуть, допСообщениеОшибки = "")
Copy link
Member

Choose a reason for hiding this comment

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

ИМХО метод ПроверитьСуществованиеФайлаКаталога не должен ничего возвращать, а просто выдавать исключение, если файла/каталога нет, вместо показа сообщения.
У нас такая семантика - методы с "Проверить..." самостоятельно выбрасывают исключение, если не выполняются необходимые условия.

Тем более, что далее в тесте Тест_ДолженЭкспортироватьНачинаяСВерсии3 и других стоит вызов этой проверки, но в результате в случае ошибки выполнение не останавливается, что неверно для теста :(

ИМХО переделать.

Возврат ОбъединитьПути(ТекущийСценарий().Каталог, "fixtures");
КонецФункции

Процедура Тест_ДолженЭкспортироватьНачинаяСВерсии3() Экспорт
Copy link
Member

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.

Код проверки после вызова Распаковщик.СинхронизироватьХранилищеКонфигурацийСГит также очень сильно похож.
Его бы также отрефакторить.


ИмяФайлаЛогаГит = ВременныеФайлы.НовоеИмяФайла("txt");

ФайлКоманды = ВременныеФайлы.НовоеИмяФайла("cmd");
Copy link
Member

Choose a reason for hiding this comment

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

ИМХО код ниже

+	ФайлКоманды = ВременныеФайлы.НовоеИмяФайла("cmd");
 +	ЗаписьФайла = Новый ЗаписьТекста(ФайлКоманды, "cp866");
 +	ЗаписьФайла.ЗаписатьСтроку("cd /d " + ОбернутьВКавычки(КаталогИсходников));
 +	ЗаписьФайла.ЗаписатьСтроку("git log --pretty=oneline >"+ОбернутьВКавычки(ИмяФайлаЛогаГит));
 +	ЗаписьФайла.Закрыть();
 +	
 +	КодВозврата = 0;
 +	ЗапуститьПриложение("cmd.exe /C " + ОбернутьВКавычки(ФайлКоманды), , Истина, КодВозврата);
 +	юТест.ПроверитьРавенство(0, КодВозврата, "Получение краткого лога хранилища git");

лучше заменить на создание объекта КомандныйФайл из 1commands, посмотри примеры в этом репо, там все просто.
Код будет выглядеть намного красивее и читабельнее, меньше технических деталей


КонецПроцедуры

Функция ВыполнитьКлонированиеТестовогоРепо()
Copy link
Member

Choose a reason for hiding this comment

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

Метод ВыполнитьКлонированиеТестовогоРепо не вызывается из другого кода.
Зачем он нужен?

Переработал модули и тесты
@ret-Phoenix
Copy link
Contributor Author

Добавил ключ -limit.
Переработал модули и тесты

@ret-Phoenix
Copy link
Contributor Author

  • Хотелось бы переработать получение параметров, чтобы устанавливались значения по умолчанию именно в них. а не в других местах.

Сейчас здесь определяются параметры
https://github.com/ret-Phoenix/gitsync/blob/d9f6a51cc5d0d75274b834193a880e057145beac/src/gitsync.os#L119-L137

Здесь проверяются default значения
https://github.com/ret-Phoenix/gitsync/blob/d9f6a51cc5d0d75274b834193a880e057145beac/src/gitsync.os#L321-L366

@artbear
Copy link
Member

artbear commented Jan 20, 2017

Имеющееся хранилище не позволяет достоверно проверить работу, т.к. там всего 3 коммита.
Если добавлять еще коммиты - надо переделывать основной тест gitsync, на что я не решился.

Предлагаю добавить еще пару коммитов в существующий файл хранилища.
Исправить свои тесты + имеющиеся тесты

@ret-Phoenix
Copy link
Contributor Author

Так и сделал, только залил больше коммитов.
Прогнал тесты, проблем не было.

Copy link
Member

@artbear artbear left a comment

Choose a reason for hiding this comment

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

Так и не исправлено поведение ПроверитьСуществованиеФайлаКаталога - нужно сразу выбрасывать исключение

Copy link
Member

Choose a reason for hiding this comment

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

Согласно указанному коду

Если Лимит <> 0 Тогда
 +		КонечнаяВерсия = ТекущаяВерсия + Лимит;
 +	КонецЕсли;
 +
 +	Если НачальнаяВерсия <> 0 Тогда
 +		Если Лимит <> 0 Тогда
 +			СледующаяВерсия = Число(НачальнаяВерсия);
 +			КонечнаяВерсия = СледующаяВерсия + Лимит;
 +		Иначе
 +			СледующаяВерсия = Макс(СледующаяВерсия, Число(НачальнаяВерсия));
 +		КонецЕсли;
 +	КонецЕсли;

видно, что нужно указывать абсолютные значения версий.

Но у нас 2 основных сценария - 1 первоначальная выгрузка и 2 очередная выгрузка
Для первого сценария нужны абсолютные значения версий.

А вот для 2го сценария "очередная выгрузка" нужны номера версий, относительно последней выгруженнной.
Какие есть идеи по реализации 2го сценария?

Copy link
Member

Choose a reason for hiding this comment

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

Ага, относительные делаются через limit.

Copy link
Contributor Author

@ret-Phoenix ret-Phoenix Jan 20, 2017

Choose a reason for hiding this comment

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

Для 2 сценария будет работать: gitsync export store -limit <СколькоТоВерсий>
иначе говоря: выгрузить не более <СколькоТоВерсий> начиная с последней выгруженной.

При:
НомерВыгруженнойВерсии = 10
gitsync export store -minversion 5 -limit 3
выгрузит поверх 10 версии: 5,6,7,8.
Т.о. опасное сочетание: minversion + limit.

При
НомерВыгруженнойВерсии = 10
МаксНомерВХранилище = 20
gitsync export store -minversion 5
выгрузит начиная с 11 все до 20.

@artbear
Copy link
Member

artbear commented Jan 20, 2017

Еще доку нужно поправить :)


ВсеТесты = Новый Массив;

// ВсеТесты.Добавить("Тест_ДолженЭкспортироватьНачинаяСВерсии3");
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.

уже )

@artbear artbear merged commit 34d7231 into oscript-library:develop Jan 21, 2017
@nixel2007
Copy link
Member

Думаю, тянет на новый релиз.

@ret-Phoenix
Copy link
Contributor Author

Тогда может прогоним режим fast #25 потом уже о релизе можно будет поговорить

@artbear
Copy link
Member

artbear commented Jan 22, 2017

Предлагаю выпустить релиз, а #25 рассмотреть уже позже

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.

3 participants