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

Feature/issue 333 #378

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Feature/issue 333 #378

wants to merge 4 commits into from

Conversation

angdev
Copy link
Member

@angdev angdev commented Oct 30, 2014

#333
#337 의 작업도 일부 병행하였습니다. 혹시 테스트되지 않은 부분이 있으면 #337 에 추가해주세요.

@angdev angdev added the verify label Oct 30, 2014
@minhyeok4dev
Copy link

확인! 너무 중복되는 create들은 따로 뽑아서 before each로 뽑아줘도 괜찮을듯~

@MoojinChae
Copy link
Contributor

확인요

@shaynekang
Copy link

음... 다른 곳에서 테스트가 하나 실패하는 것 같네요. ㅠㅠ

Failures:

  1) Admin::Messages::SendMessageService should fail if notice is invalid
     Failure/Error: expect(out[:status]).to eq(:failure)

       expected: :failure
            got: :success

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -:failure
       +:success

     # ./spec/unit/services/admin/messages/send_message_service_spec.rb:40:in `block (2 levels) in <top (required)>'
     # ./spec/rails_helper.rb:40:in `block (3 levels) in <top (required)>'
     # ./spec/rails_helper.rb:39:in `block (2 levels) in <top (required)>'

Finished in 18.22 seconds (files took 6.56 seconds to load)
138 examples, 1 failure

Failed examples:

rspec ./spec/unit/services/admin/messages/send_message_service_spec.rb:37 # Admin::Messages::SendMessageService should fail if notice is invalid

수정이 필요할 것 같습니다~

@shaynekang
Copy link

그리고 단위테스트는 Spec(기술 명세서)의 역할도 하기 때문에, 다소 코드가 중복되더라도 한 눈에 읽기 쉽게 작성하는게 좋습니다. ㅎㅎ

spec/unit/models/notice_spec.rb의 메타프로그래밍 scope 부분은 전 오히려 코드가 중복되도 상관 없으니, 좀 더 읽기 쉽도록 풀어 쓰는게 좋지 않을까 생각합니다. ㅎㅎ

@yhoonkim
Copy link
Contributor

yhoonkim commented Nov 3, 2014

멘토님이 말씀하신 에러가 다른 브랜치 #376 에서도 발생했었어요.
나머진 확인!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants