Skip to content

Conversation

@gmponos
Copy link
Contributor

@gmponos gmponos commented Dec 8, 2018

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
License MIT

What's in this PR?

  • Use assertSame where it had meaning.
  • Use a clone of the objects under testing. This way we ensure that the object has not changed state.
  • Added some more assertions.

To Do

  • Need to check more places about using assertEqual with a clone.

@gmponos
Copy link
Contributor Author

gmponos commented Dec 8, 2018

I think that's it.. hope I haven't missed anything but if I did we can do more improvements later.

@gmponos
Copy link
Contributor Author

gmponos commented Dec 9, 2018

I also added more invalid cases of METHOD... see

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

i like the changes to assertSame and additional type checks.

i am not sure i fully get what the getClone method is supposed to do - i think it would be cleaner to explicitly clone the original request/response in 1-2 places where we test that indeed the original object is not altered, instead of having a clone method and that many checks? the getCone method name is also very generic and not explaining what it is for.

@gmponos
Copy link
Contributor Author

gmponos commented Dec 12, 2018

i am not sure i fully get what the getClone method is supposed to do

The abstract definition of getClone method is there in order for the trait to ensure that the method exists and be able to be used by the trait tests.. check this

i think it would be cleaner to explicitly clone the original request/response in 1-2 places

Yes I can clone the message per test case... but the places I counted are 5 not 1-2 😄 .. I have no disagreement about this.. I will make the changes...

the getCone method name is also very generic

I was pretty sure about this comment.. I have the same feeling as you... Since I will remove it and do the clone per test case then no need to think of a name.. if you change your mind and keep it I vote for getMessageOriginalState

@dbu
Copy link
Contributor

dbu commented Dec 12, 2018

okay, cool! when i said 1-2 places i was thinking if we need the clone in all places we currently use it. if there is a specific reason to test that in each of the places, i am all for it, but we should not test the same thing multiple times.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

i find it more readable this way with the clone in the tests. thanks!

@dbu dbu merged commit d2b5960 into php-http:master Dec 13, 2018
@gmponos gmponos deleted the improve_tests branch December 13, 2018 09:48
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.

2 participants