Fix assert conflicts in `test\Unit` adding `assetNotIdentical()` #793

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
Contributor

jails commented Jan 20, 2013

  • assertXX & assertNotXX should share the same code base.
  • consistent API for assertNull() / assetNotNull().
  • adding missing assetNotIdentical().
  • normalize line endings.
  • typo fix: renaming $actual to $result all way long
  • remove assertRegExp(), conflict with assertPattern().
  • remove assertNotRegExp, conflict with assertNoPattern().
  • remove assertEmpty(), conflict with assertFalse().
  • remove assertNotEmpty(), conflict with assertTrue().
Member

blainesch commented Jan 21, 2013

Why does assertTrue check empty instead of bool? I'd say true should check true and empty should check empty. This might break backwards compatibility but it's thrown me off already.

Member

marcghorayeb commented Jan 21, 2013

I'm with @blainesch on assertTrue, it should be a strict test. Empty is way too generic.

Contributor

gwoo commented Jan 21, 2013

assertTrue has been used to check the existence of any value, not strictly whether it is boolean true. This was done so we did not have to know a keep a large assertion api in our head. Anything that changes that would be a BC break, so we should be very careful.

Member

marcghorayeb commented Jan 21, 2013

@gwoo Maybe we could change assertTrue to assertIsset or something else then, it's a little confusing right now for outsiders :)

Member

nervetattoo commented Jan 21, 2013

👍 on changing assertTrue.
It should definitively be a strict test or perhaps named assertTruthy.
Although it would be a BC change isn't it better to have that before 1.0 happens?

Upvote

Contributor

brandonwestcott commented Jan 21, 2013

Agreed, assertTrue should check the boolean.

Contributor

buddylindsey commented Jan 21, 2013

I like the idea of assertIsSet over assertTrue, and setting assertTrue to check the value. The explicitness is nice. If you break BC pre-1.0 that is better than breaking it after.

Member

rapzo commented Jan 21, 2013

I agree with keeping it with a small api :-)

Member

d1rk commented Jan 22, 2013

👍

Contributor

jails commented Jan 22, 2013

I think the discussion cannot be restricted to assertTrue/assertFalse. Currently the API is in an "hybrid state" and I think there only 5 possible viable states:

1 - reverting back to the small API
2 - point 1 + BC Break assertTrue/assertFalse (since the li3 unit test framework is independant, we can BC break what we want...)
3 - adding only PHPUnit asserts which doesn't conflicts the current API. (this PR)
4 - point 3 + BC Break assertTrue/assertFalse
5 - completly move on to PHPUnit asserts and keep consistent with it (which mean BC break the current API). But does supporting PHPUnit asserts is great deal of interest ?

I would prefer to keep a small API in the core but I'm also fine to BC break all the current API in flavor of PHPUnit asserts if necessary. As long as it move on, whatever.

Contributor

jails commented Feb 8, 2013

Maybe we should go ahead on this and fix this thing once.

Member

marcghorayeb commented Feb 10, 2013

I'm for keeping a small API too, but we have to be strict on the true / false test i reckon
what would it take to move to phpunit ? i'm against reinventing the wheel if phpunit's framework is light and easy to use

Contributor

mehlah commented Feb 10, 2013

Oh my, the party is here?
I would ❤️ to have a small and compact built-in API to Lithium, and a bunch of libraries that replace/extend the whole Lithium test framework. Something like RSpec vs TestUnit vs MiniTest for Rubyists (https://www.ruby-toolbox.com/categories/testing_frameworks.html)...
We should support libs like li3_mockery, li3_phpUnit, li3_atoum, li3_behat when it makes sense and needed, instead of providing the same thing again and again.

Contributor

jails commented Feb 10, 2013

Agreed ! 👍

Contributor

gwoo commented Feb 10, 2013

@jails so that would be point 4 from your previous comment?

Contributor

gwoo commented Feb 10, 2013

@marcghorayeb phpunit is at least 2 times te size of all lithium and code coverage, web ui, and other things are all extra.

Contributor

jails commented Feb 10, 2013

@gwoo I think the point 1 seems more consistent. the current test API was enough for testing the core until now and like @mehlah said there's a lots of unit test framework over there. Imo the choice of the test framework is mostly a matter of taste and the core should only integrate the asserts needed to build its tests. All higher level of abstraction (i.e. not necessary for the core) should be delegated to external libraries imo.

Contributor

gwoo commented Feb 10, 2013

Ok cool. Happy to follow the original approach.

Owner

davidpersson commented Feb 10, 2013

I'd vote for point 2 as it would - in the end - lead to a well defined API. I'm with @gwoo here, we should follow the way of the original approach.

I've alway liked the idea to be compatible with other major testing frameworks - namely phpunit. There aren't many reasons not to.

This compatibility should make the signature and the behavior - as closely as it makes sense - of the originally present assertion methods the same as the corresponding ones in phpunit.

In that sense I'd like to see more strict assertTrue/assertFalse behavior. I think we could lessen the impact of the BC break if we can mark test case classes as being "strict" or not. However I'm currently not sure if this wouldn't lead down the wrong path and it'd be better to not provide such a switch.

Contributor

gwoo commented Feb 10, 2013

So, add assertNotEmpty() and make assertTrue/False strictly boolean?

Owner

davidpersson commented Feb 10, 2013

Changing the behavior of the assertTrue/assertFalse methods to increase compatibility will only lead us to more questions like the ones about having assertEquals/assertSame instead of assertEqual/assertIdentical. I think this is a problem which should be looked at separately and given a lower priority. For now I vote for reverting to the simple API and creating another PR for raising the compatibility with phpunit by making assertTrue/assertFalse more strict and fixing a few other things.

Contributor

buddylindsey commented Feb 10, 2013

I use lithium everyday at work. In the past doing php development, I have used phpunit, and prefer it. Coming to lithium was kind of a shock that the test framework was re-written, so it leaves a lot to be desired tooling wise. I have worked on a couple of things here and there to extend it, specifically with doing Jenkins CI, but it took quite a bit of time. Whereas if I could just use something like li3_phpunit for testing and code coverage things would have been peachy in 20 minutes.

I may have missed it in the conversation, but what does it take to get to where we can do an li3_phpunit?

Contributor

jails commented Feb 10, 2013

Agree with @davidpersson disputing about the API compatibility on assertTrue assertFalse assertEquals and assertSame should be considered once the small API has been reverted imo.

Member

blainesch commented Feb 12, 2013

@davidpersson I don't think we are making assertTrue and assertFalse stricter to be more compatible with phpunit, it should just do exactly what it says it does. Imo this makes tests easier to read.

Starting after this pr imo we should make assertTrue/assertFalse more strict and add back assertEmpty/assertNotEmpty like @gwoo said.

Contributor

jails commented Feb 12, 2013

Imo integrating something in the core should be motivated by some of the following points:

  • the core need it for going ahead.
  • the feature is awesome enough to be considered as a "de facto" standard.

Concerning tests, I think the core should only provide a way for people to use their own test framework.
No need to reinventing the wheel here (even more if it's not for a better wheel).

Contributor

fellars commented Feb 13, 2013

I’ve only passively followed this thread, so not sure if already discussed, but what about adding a third parameter $strict=false to assertTrue/assertFalse. If $strict === true, then perform a strict comparison, but defaults to false to not break BC.

It may be a nice middle ground between options.

From: Blaine Schmeisser [mailto:notifications@github.com]
Sent: Tuesday, February 12, 2013 3:44 PM
To: UnionOfRAD/lithium
Subject: Re: [lithium] Fix assert conflicts in test\Unit adding assetNotIdentical() (#793)

@davidpersson https://github.com/davidpersson I don't think we are making assertTrue and assertFalse stricter to be more compatible with phpunit, it should just do exactly what it says it does. Imo this makes tests easier to read.

Starting after this pr imo we should make assertTrue/assertFalse more strict and add back assertEmpty/assertNotEmpty like @gwoo https://github.com/gwoo said.


Reply to this email directly or view it on GitHub #793 (comment) .

https://github.com/notifications/beacon/8N2Ngwp4E4s46ItXC5cbBnBSl6v_jnW0CPmr0Z6vhTNVLZqLxCqxx2ijfUXTHSP6.gif

Contributor

buddylindsey commented Feb 15, 2013

Just thought I would mention this here I created an li3_phpunit. I am more of a fan of PHPUnit over the built-in testing features. So off and on over the last week I created a plug-in so I can start using PHPUnit instead. Still has a lot of work, but is at least a working proof of concept at this point.

Contributor

jails commented Feb 15, 2013

Awesome 👍

@jails jails closed this Feb 27, 2013

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