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

Remove usage of `should` in test method names betterspecs.org/#should #42

Merged
merged 1 commit into from Oct 16, 2014

Conversation

Projects
None yet
7 participants
@henrikbjorn
Contributor

henrikbjorn commented Sep 8, 2014

It seems like the test uses a lot of should instead of just stating what it does, and what the purpose of the test is.

betterspecs.org/#should is properly the best description to why should is disallowed. Also its a best practice to not use should when speccing .

@Miliooo

View changes

Show outdated Hide outdated test/Broadway/Domain/DomainMessageTest.php
@Miliooo

This comment has been minimized.

Show comment
Hide comment
@Miliooo

Miliooo Sep 8, 2014

Contributor

👍

Contributor

Miliooo commented Sep 8, 2014

👍

@wjzijderveld

This comment has been minimized.

Show comment
Hide comment
@wjzijderveld

wjzijderveld Sep 16, 2014

Member

+1 @henrikbjorn if you could rebase and update the it_have to it_has

Member

wjzijderveld commented Sep 16, 2014

+1 @henrikbjorn if you could rebase and update the it_have to it_has

@henrikbjorn

This comment has been minimized.

Show comment
Hide comment
@henrikbjorn

henrikbjorn Sep 17, 2014

Contributor

@wjzijderveld currently on an island in the South China Sea, but when i get home in two weeks, sure :)

Contributor

henrikbjorn commented Sep 17, 2014

@wjzijderveld currently on an island in the South China Sea, but when i get home in two weeks, sure :)

@kelvinj

This comment has been minimized.

Show comment
Hide comment
@kelvinj

kelvinj Sep 17, 2014

Contributor

@henrikbjorn pfff … where's the commitment

Contributor

kelvinj commented Sep 17, 2014

@henrikbjorn pfff … where's the commitment

@Miliooo

This comment has been minimized.

Show comment
Hide comment
@Miliooo

Miliooo Sep 17, 2014

Contributor

I guess he likes the ☀️ and 🍉 more 😢

You should update this pr
1 test failed

Contributor

Miliooo commented Sep 17, 2014

I guess he likes the ☀️ and 🍉 more 😢

You should update this pr
1 test failed

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Sep 17, 2014

You should update this pr

😆😂

sstok commented Sep 17, 2014

You should update this pr

😆😂

@@ -23,7 +23,7 @@
/**
* @test
*/
public function it_should_be_serializable()
public function its_serializable()

This comment has been minimized.

@cordoval

cordoval Sep 17, 2014

Contributor

it_serializes is better in this case i think, or rather conjugate it it_gets_serialized

@cordoval

cordoval Sep 17, 2014

Contributor

it_serializes is better in this case i think, or rather conjugate it it_gets_serialized

This comment has been minimized.

@stof

stof Sep 17, 2014

no. The goal is checking that the class is serializable, not that it serializes something. It is not the same

@stof

stof Sep 17, 2014

no. The goal is checking that the class is serializable, not that it serializes something. It is not the same

This comment has been minimized.

@cordoval

cordoval Sep 17, 2014

Contributor

that is why i ended up with it_gets_serialized @stof

I believe its is more for its_x_behaves_in_this_way()

@cordoval

cordoval Sep 17, 2014

Contributor

that is why i ended up with it_gets_serialized @stof

I believe its is more for its_x_behaves_in_this_way()

This comment has been minimized.

@stof

stof Sep 17, 2014

but the spec does not check that it gets serialized at all. What it checks is that it is of type SerializableInterface. So IMO, it_is_serializable is still the best name for what it does

@stof

stof Sep 17, 2014

but the spec does not check that it gets serialized at all. What it checks is that it is of type SerializableInterface. So IMO, it_is_serializable is still the best name for what it does

This comment has been minimized.

@cordoval

cordoval Sep 17, 2014

Contributor

that is type of SerializableInterface means exactly that it has that capability of being serializable. it_is_serializable or it_gets_serialized (+1) is better than the current state of its_serialized. yeap

@cordoval

cordoval Sep 17, 2014

Contributor

that is type of SerializableInterface means exactly that it has that capability of being serializable. it_is_serializable or it_gets_serialized (+1) is better than the current state of its_serialized. yeap

This comment has been minimized.

@Miliooo

Miliooo Sep 17, 2014

Contributor

or it_implements_the_interface 💛

@Miliooo

Miliooo Sep 17, 2014

Contributor

or it_implements_the_interface 💛

This comment has been minimized.

@cordoval

cordoval Sep 17, 2014

Contributor

that is even worst, that is why focus on behavior and not on devices of the language

@cordoval

cordoval Sep 17, 2014

Contributor

that is even worst, that is why focus on behavior and not on devices of the language

@@ -50,7 +50,7 @@ public function setUp()
/**
* @test
*/
public function it_should_add_the_command_class_and_arguments()
public function it_adds_the_command_class_and_arguments()

This comment has been minimized.

@cordoval

cordoval Sep 17, 2014

Contributor

like here 👍 you conjugate it

@cordoval

cordoval Sep 17, 2014

Contributor

like here 👍 you conjugate it

@henrikbjorn

This comment has been minimized.

Show comment
Hide comment
@henrikbjorn

henrikbjorn Sep 27, 2014

Contributor

@wjzijderveld @sstok etc. it is now rebased from the corners of the South China Sea :) Enough commitment? 🌞

Contributor

henrikbjorn commented Sep 27, 2014

@wjzijderveld @sstok etc. it is now rebased from the corners of the South China Sea :) Enough commitment? 🌞

@wjzijderveld

This comment has been minimized.

Show comment
Hide comment
@wjzijderveld

wjzijderveld Sep 29, 2014

Member

👍 after you update the TestCase :) Namespace has changed since you opened this PR 😁

Just enough commitment 😁

Member

wjzijderveld commented Sep 29, 2014

👍 after you update the TestCase :) Namespace has changed since you opened this PR 😁

Just enough commitment 😁

@wjzijderveld

This comment has been minimized.

Show comment
Hide comment
@wjzijderveld

wjzijderveld Sep 29, 2014

Member

Sorry, didn't look closely enough..
You can remove the UUID TestCase completely, it got moved to a new repository https://github.com/qandidate-labs/broadway-uuid-generator

Member

wjzijderveld commented Sep 29, 2014

Sorry, didn't look closely enough..
You can remove the UUID TestCase completely, it got moved to a new repository https://github.com/qandidate-labs/broadway-uuid-generator

@henrikbjorn

This comment has been minimized.

Show comment
Hide comment
@henrikbjorn

henrikbjorn Sep 29, 2014

Contributor

What is the new namespace?

Contributor

henrikbjorn commented Sep 29, 2014

What is the new namespace?

@wjzijderveld

This comment has been minimized.

Show comment
Hide comment
@wjzijderveld

wjzijderveld Oct 6, 2014

Member

@henrikbjorn Only just saw your comment... Github didn't email me 😞

You can remove the TestCase completely, that class isn't part of this repository anymore, as it now lives here: https://github.com/qandidate-labs/broadway-uuid-generator/blob/58380b723c671807131fe9ac0ff077f162be47bb/test/Broadway/UuidGenerator/Rfc4122/Version4GeneratorTest.php

Member

wjzijderveld commented Oct 6, 2014

@henrikbjorn Only just saw your comment... Github didn't email me 😞

You can remove the TestCase completely, that class isn't part of this repository anymore, as it now lives here: https://github.com/qandidate-labs/broadway-uuid-generator/blob/58380b723c671807131fe9ac0ff077f162be47bb/test/Broadway/UuidGenerator/Rfc4122/Version4GeneratorTest.php

wjzijderveld added a commit that referenced this pull request Oct 16, 2014

Merge pull request #42 from henrikbjorn/remove-should-from-tests
Remove usage of `should` in test method names betterspecs.org/#should

@wjzijderveld wjzijderveld merged commit 58e50ed into broadway:master Oct 16, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@wjzijderveld

This comment has been minimized.

Show comment
Hide comment
@wjzijderveld

wjzijderveld Oct 16, 2014

Member

Thanks! Awesome commitment from the South Chinese Sea 😄

Member

wjzijderveld commented Oct 16, 2014

Thanks! Awesome commitment from the South Chinese Sea 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment