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

Specs for Resolve #4

Closed
wants to merge 98 commits into from
Closed

Specs for Resolve #4

wants to merge 98 commits into from

Conversation

SwinX
Copy link

@SwinX SwinX commented Jun 8, 2015

Resolved #3

Added basic tests covering input params processing, dependency ordering and exceptions on retain cycles.

SwinX added 5 commits June 5, 2015 12:39
…est (jshint does not allow unused params). Removed 'common' context from tests.
…r to tests run correctly. Implemented basic tests for resolve method - covering input params checking behavior, blocks include order, exceptions on cyclic dependencies detection
@SwinX
Copy link
Author

SwinX commented Jun 8, 2015

Resolve comment was not included since this PR is not resolving #3 completely

SwinX added 19 commits June 9, 2015 11:30
…ch in entity can depend on another entity. Added spec describing tech in entity can depend on tech in another entity
…. Added tests about ignorance of tech dependencies if tech is not specified and about tech dependency for specified tech must be placed into entities list
@@ -0,0 +1,7 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Такие коментарии не нужны.

@blond
Copy link
Member

blond commented Jun 10, 2015

Все тесты нужно разделить по группам, чтобы удобнее было читать:

  • про обработку ошибоку
  • про сборку без технологий
  • про технологии
  • про порядок, который надо сохранить
  • про приоритеты
  • и т.д.

deps = [
{
entity: { block: 'C' },
dependOn: {
Copy link
Member

Choose a reason for hiding this comment

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

Тут должен быть всегда массив.

@SwinX
Copy link
Author

SwinX commented Jun 19, 2015

Переписал тесты на новый ассертер chai, разбил тесты про рекомендуемый и явный порядок на две группы, дополнил разные группы тестов новыми тестами, подшаманил читаемость тестов о порядке включения. Нужно ещё одно ревью от @blond

"matcha": "0.6.0",
"mocha": "2.2.5",
"must": "0.12.0"
"chai": "3.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Напоминаю, что зависимости лучше ставить через npm i chai -D, чтобы порядок в package.json был правильный.

@blond
Copy link
Member

blond commented Jun 20, 2015

После chai стало читаться лучше.

Написал немного замечаний. Становится сложно ревьювить из-за кол-ва кейсов: сложно понять каких кейсов не хватает.

Давай начнём думать на какие разделы поделить кейсы. Можно оформить отдельными задачими. На каждый такой раздел свой отдельный PR. Начинать лучше с самых простой и очевидной группы кейсов.

@blond blond changed the title Issue 3 Specs for Resolve Jun 21, 2015
@blond blond removed their assignment Jun 21, 2015
@blond blond added the ready label Jun 21, 2015
@SwinX
Copy link
Author

SwinX commented Jul 7, 2015

All tests from this PR were already merged in another PRs.
Closed.

@SwinX SwinX closed this Jul 7, 2015
@SwinX SwinX removed the ready label Jul 7, 2015
@blond blond deleted the issue-3 branch December 2, 2015 22:22
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.

None yet

3 participants