UserActivationServiceImpl.findOlderThan() unit tests #558

Shagbark · 2017-03-30T21:49:46Z

Addressed to #337

php-coder · 2017-03-30T21:57:21Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

@@ -17,6 +17,7 @@
 */
 package ru.mystamps.web.service

+import org.apache.commons.lang3.time.DateUtils


Откати эту часть с импортом, тут уже был правильный импорт, а это, видимо, IDEA постаралась.

php-coder · 2017-03-30T21:58:29Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

+	// Tests for findOlderThan()
+	//
+
+	def "findOlderThan() should throw exception when passed days are less zero"() {


s/less zero/less than zero/

s/passed days/days/

php-coder · 2017-03-30T22:00:39Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

+
+	def "findOlderThan() should invoke dao, pass changed date and return the result "() {
+		given:
+			def days = 4


Укажи тип переменной явно.

А параметризованный тест нужно добавить?
Или одного значения хватит?

Я думал об этом, но решил, что и так должно быть достаточно. Поэтому оставлю это на твое усмотрение -- хочешь попробовать -- попробуй, если нет, то оставим как есть.

php-coder · 2017-03-30T22:02:02Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

+		given:
+			def days = 4
+		and:
+			def expectedDate = DateUtils.addDays(new Date(), -days)


Date expectedDate = new Date() + days

php-coder · 2017-03-30T22:03:13Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

+			thrown IllegalArgumentException
+	}
+
+	def "findOlderThan() should invoke dao, pass changed date and return the result "() {


Ты не проверяешь, что метод возвращает значение от дао. Добавишь проверку?

php-coder · 2017-03-30T22:05:55Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

@@ -264,5 +264,30 @@ class UsersActivationServiceImplTest extends Specification {
 		then:
 			1 * usersActivationDao.removeByActivationKey(activationKey)
 	}
+
+	//
+	// Tests for findOlderThan()


Передвинь эти тесты повыше, так чтобы порядок тестируемых методов в тестах совпадал с их порядком объявления этих методов в самом сервисе.

Shagbark · 2017-03-30T22:06:53Z

Да, сейчас добавлю.
И, наверное, там будет
Date expectedDate = new Date() - days? Не, плюс?

php-coder · 2017-03-30T22:09:08Z

Date expectedDate = new Date() - days? Не, плюс?

Да, разумеется.

php-coder · 2017-03-30T22:14:13Z

P.S. Там на код CodeNarc ругается, так что надо добавить пару аннотаций (как и в других тестах), чтобы отключить эту ругань. В отчете трэвиса можешь посмотреть подробности.

Shagbark · 2017-03-30T22:23:09Z

Я так понимаю, что CodeNarc ругается на то, что мы делаем замыкание, а он хочет, чтобы в метод передали параметр?
А что если мы уберем круглые скобки при вызове метода?
1* userActivationDao.findOlderThan{ Date date -> .... }

php-coder · 2017-03-30T22:24:14Z

Ты можешь запустить его локально и проверить: https://github.com/php-coder/mystamps/wiki/codenarc

Shagbark · 2017-03-30T22:30:26Z

Хм, странно.
При убранной аннотации @SuppressWarnings и с оставленными круглыми скобками, он мне выдает
Build Success.
И тоже самое с аннотацией, и даже если не стоят круглые скобки.

php-coder · 2017-03-30T22:34:21Z

Я спать 😴 Если останутся вопросы -- задавай, как будет время, посмотрю.

Shagbark · 2017-03-30T22:34:54Z

Спокойной ночи :)

Shagbark · 2017-04-03T19:30:20Z

Вячеслав, а можно запустить проверку на Travisе без коммита?

php-coder · 2017-04-03T20:38:49Z

Вячеслав, а можно запустить проверку на Travisе без коммита?

Можно. Все команды, которые выполняются в Travis описаны в его конфигурационном файле:

mystamps/.travis.yml

Lines 11 to 35 in 88979cd

    
           before_script: 
        
             - if [ "$SPRING_PROFILES_ACTIVE" = 'travis' ]; then 
        
                 mysql -u travis -e 'CREATE DATABASE mystamps CHARACTER SET utf8;'; 
        
                 if [ "$TRAVIS_BRANCH" = 'prod' ]; then 
        
                   pip install --user ansible==2.1.1.0; 
        
                 fi; 
        
                 npm install -g bootlint; 
        
                 pip install --user html5validator; 
        
                 pip install --user robotframework-lint; 
        
                 if [ "$TRAVIS_PULL_REQUEST" != 'false' ]; then 
        
                   gem install danger nokogiri --no-ri --no-rdoc; 
        
                 fi; 
        
               fi 
        
           script: 
        
             - if [ "$SPRING_PROFILES_ACTIVE" = 'travis' ]; then 
        
                 ./src/main/scripts/ci/check-build-and-verify.sh; 
        
               else 
        
                 ./src/main/scripts/ci/check-build-and-verify.sh --only-integration-tests; 
        
               fi 
        
           after_success: 
        
             - if [ "$SPRING_PROFILES_ACTIVE" = 'travis' ]; then 
        
                 ./src/main/scripts/ci/publish-code-coverage.sh; 
        
               fi

Но у тебя падает сейчас только CodeNarc и запустить только его локально довольно просто: https://github.com/php-coder/mystamps/wiki/codenarc

php-coder · 2017-04-03T20:40:43Z

P.S. Недочитал, видио. На трэвисе без коммита, наверное, могу только я, перезапустив задачу. В твоем случае придется делать коммит. Либо переписывать существующий. А зачем тебе это, к слову?

Shagbark · 2017-04-03T20:47:27Z

Да у меня, просто, CodeNar всегда выдает на консоли Build Success (а в отчете пусто). А также кидает такой exception:
java.lang.AssertionError: Invalid argument format: [Stamps]. Expression: matcher. Values: matcher = java.util.regex.Matcher[pattern=\-(.*)\=(.*) region=0,6 lastmatch=]

А в Travise он пишет, что именно не так. Как вот сейчас, что лучше использовать [], вместо new ArrayList<>(). Вот и спросил.

php-coder · 2017-04-03T20:50:55Z

А также кидает такой exception:
java.lang.AssertionError: Invalid argument format: [Stamps]. Expression: matcher. Values: matcher = java.util.regex.Matcher[pattern=-(.)=(.) region=0,6 lastmatch=]

Не удивляюсь, если это баг в самом плагине :( А можешь полное сообщение об ошибке привести (например, сделать скриншот)?

Shagbark · 2017-04-03T20:56:14Z

Shagbark · 2017-04-03T20:56:52Z

Вот окончание ошибки

php-coder · 2017-04-03T20:59:31Z

Похоже на багу либо в плагине либо в самом CodeNarc, который не может корректно обрабатывать пути с пробелами. Попробуй скопировать проект в каталог в пути к которому нет пробелов и запустить проверку там.

Shagbark · 2017-04-03T21:02:55Z

Неа, такую же ошибку выдает :(

php-coder · 2017-04-03T21:08:41Z

Тогда делай любые коммиты, все равно потом сквошить в один нужно будет :)

P.S. git commit --allow-empty -m 'Tada'

Shagbark · 2017-04-03T21:09:02Z

Хорошо :)

Shagbark · 2017-04-03T22:10:29Z

Хм, странно, а почему сейчас не проходит build?
Из-за этого?

php-coder · 2017-04-04T08:30:58Z

Нет, из-за бага в danger который я как раз с разработчиками обсуждаю.

Можешь засквошить коммиты в один и желательно в авторстве указать имя с фамилией, а не просто имя или нет. Впрочем, если ты не хочешь раскрывать свою фамилию, то это не обязательно.

Shagbark · 2017-04-04T10:51:02Z

Я попробую :)
Никогда не сквошил коммиты в один.
Почитаю и вечером попробую сделать.

php-coder · 2017-04-05T10:33:08Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

@@ -264,5 +295,5 @@ class UsersActivationServiceImplTest extends Specification {
 		then:
 			1 * usersActivationDao.removeByActivationKey(activationKey)
 	}
-	
+


Please, remove this useless modification.

php-coder · 2017-04-05T10:33:52Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

@@ -17,6 +17,8 @@
 */
 package ru.mystamps.web.service

+import ru.mystamps.web.dao.dto.UsersActivationFullDto
+import ru.mystamps.web.tests.DateUtils


Please, move these imports below, to other import from ru.mystamps package.

php-coder · 2017-04-05T10:36:03Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

+		and:
+			Date expectedDate = new Date() - days
+		and:
+			List<UsersActivationFullDto> expectedResult = []


Let's compare with non-empty list:

List<UsersActivationFullDto> expectedResult = [ TestObjects.createUsersActivationFullDto() ]

php-coder · 2017-04-05T10:36:43Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

+	}
+
+	@SuppressWarnings(['ClosureAsLastMethodParameter', 'UnnecessaryReturnKeyword'])
+	def "findOlderThan() should invoke dao, pass changed date and return the result "() {


and return the result "

Please, remove trailing space at the end of the method name.

php-coder · 2017-04-05T10:37:37Z

и желательно в авторстве указать имя с фамилией, а не просто имя или нет. Впрочем, если ты не хочешь раскрывать свою фамилию, то это не обязательно.

What did you decide on that?

Shagbark · 2017-04-05T11:26:53Z

Я указал в commit message имя и фамилию :)
Дмитрий Процко (Dmitry Protsko).

php-coder · 2017-04-05T11:36:10Z

It's shouldn't be in the message itself but in the author's info:

$ git config --global user.name "Your Name"
$ git commit --am --reset-author

Shagbark · 2017-04-05T11:37:33Z

Аа, понял, сделаю тогда сегодня.

php-coder · 2017-04-05T11:41:40Z

Thanks! And the last thing, please, set the following commit message:

UsersActivationService.findOlderThan(): add unit tests.

Fix #337

Shagbark · 2017-04-05T11:44:09Z

Хорошо.
А коммиты также сквошить?

php-coder · 2017-04-05T12:32:22Z

Yes, I'm expecting a single commit with the provided message.

mystamps-bot · 2017-04-06T19:17:41Z

	1 Message
📖	@Shagbark thank you for the PR! All quality checks have been passed! Next step is to wait when @php-coder will review this code

Generated by 🚫 Danger

coveralls · 2017-04-06T19:18:07Z

Changes Unknown when pulling 21f6580 on Shagbark:gh337_findOlderThan_utests into ** on php-coder:master**.

Shagbark · 2017-04-06T19:22:32Z

Блин, я же поменял user.name в git config :(

php-coder · 2017-04-06T20:56:55Z

@Shagbark Еще не поздно :)

php-coder · 2017-04-06T20:59:42Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

 import ru.mystamps.web.validation.ValidationRules
+import ru.mystamps.web.dao.dto.UsersActivationFullDto
+import ru.mystamps.web.tests.DateUtils


Старайся уменьшать размер диффа и не меняй ничего, что не относится напрямую к твоему изменению. Так и ревьювить проще, и патч меньше, и ребэйзы проходят с меньшей вероятностью пересечься с другими изменениями и откатывать, в случае чего, тоже проще.

В данном случае, верни, пожалуйста, импорт ru.mystamps.web.tests.DateUtils туда где он был. А импорт ru.mystamps.web.dao.dto.UsersActivationFullDto должен быть после импорта import ru.mystamps.web.dao.dto.UsersActivationDto

Я так понимаю, мне нужно сделать git checkout --UserActivationServiceImpl.java до определенного коммита, в котором не было изменений, которые не относятся к диффу (либо до начала ветвления)?
А потом добавить 2 теста, с учетом поправок?

Я ни разу не отменял коммит, поэтому спрашиваю :)

It's good that you're asking because I'm expecting that you will edit a file and will execute git commit -a --amend to add your modifications to the existing commit. Note that existing commit will be overwritten and to push it to the server you will have to use git push -f.

php-coder · 2017-04-06T21:00:35Z

src/test/groovy/ru/mystamps/web/service/UsersActivationServiceImplTest.groovy

@@ -264,5 +295,4 @@ class UsersActivationServiceImplTest extends Specification {
 		then:
 			1 * usersActivationDao.removeByActivationKey(activationKey)
 	}
-


См. выше. Коммит не должен содержать ничего лишнего и увеличивать энтропию.

А как можно убрать вот эту строку?
Я сделал git commit -a --amend, убрал изменения с импортами, а здесь изменения остались?

Для этого ее нужно добавить (вернуть назад) :)

Аа, дурак, понял.
Слушай, а такой вопрос: я сделал git commit -a --amend -am --reset-author, а все равно запушилось как Shagbark?

Я вижу, что запушилось все ок. Спасибо!

Дай знать как эту строчку победишь :)

Хорошо)

Ура! Я победил :)

Fix php-coder#337

php-coder · 2017-04-16T18:22:23Z

Отлично! Спасибо!

php-coder · 2017-04-16T18:28:54Z

Merged in 15e0625 commit.

coveralls · 2017-04-16T19:14:50Z

Changes Unknown when pulling d59208f on Shagbark:gh337_findOlderThan_utests into ** on php-coder:master**.

php-coder · 2017-04-25T18:10:29Z

Looks like I forgot to give you this link: https://github.com/php-coder/mystamps/wiki/after_merge_steps

php-coder added the in progress label Mar 30, 2017

php-coder reviewed Mar 30, 2017

View reviewed changes

php-coder changed the title ~~Gh337 findOlderThan utests~~ UserActivationServiceImpl.findOlderThan() unit tests Mar 31, 2017

Shagbark force-pushed the gh337_findOlderThan_utests branch from 35534f9 to cd354e4 Compare April 4, 2017 20:20

php-coder reviewed Apr 5, 2017

View reviewed changes

php-coder force-pushed the master branch from 7dca59c to 1410b5a Compare April 5, 2017 22:27

Shagbark force-pushed the gh337_findOlderThan_utests branch from cd354e4 to 21f6580 Compare April 6, 2017 19:04

php-coder reviewed Apr 6, 2017

View reviewed changes

Shagbark force-pushed the gh337_findOlderThan_utests branch from 21f6580 to 22972d0 Compare April 16, 2017 14:48

UsersActivationService.findOlderThan(): add unit tests.

d59208f

Fix php-coder#337

Shagbark force-pushed the gh337_findOlderThan_utests branch from 22972d0 to d59208f Compare April 16, 2017 18:16

php-coder closed this Apr 16, 2017

php-coder added resolution/merged and removed in progress labels Apr 16, 2017

UserActivationServiceImpl.findOlderThan() unit tests #558

UserActivationServiceImpl.findOlderThan() unit tests #558

Uh oh!

Conversation

Shagbark commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shagbark commented Mar 30, 2017

Uh oh!

php-coder commented Mar 30, 2017

Uh oh!

php-coder commented Mar 30, 2017

Uh oh!

Shagbark commented Mar 30, 2017

Uh oh!

php-coder commented Mar 30, 2017

Uh oh!

Shagbark commented Mar 30, 2017

Uh oh!

php-coder commented Mar 30, 2017

Uh oh!

Shagbark commented Mar 30, 2017

Uh oh!

Shagbark commented Apr 3, 2017

Uh oh!

php-coder commented Apr 3, 2017

Uh oh!

php-coder commented Apr 3, 2017

Uh oh!

Shagbark commented Apr 3, 2017

Uh oh!

php-coder commented Apr 3, 2017

Uh oh!

Shagbark commented Apr 3, 2017

Uh oh!

Shagbark commented Apr 3, 2017

Uh oh!

php-coder commented Apr 3, 2017

Uh oh!

Shagbark commented Apr 3, 2017

Uh oh!

php-coder commented Apr 3, 2017

Uh oh!

Shagbark commented Apr 3, 2017

Uh oh!

Shagbark commented Apr 3, 2017

Uh oh!

php-coder commented Apr 4, 2017

Uh oh!

Shagbark commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder Mar 30, 2017 •

edited

Loading

php-coder Apr 5, 2017 •

edited

Loading

php-coder commented Apr 5, 2017 •

edited

Loading

mystamps-bot commented Apr 6, 2017 •

edited

Loading