? ? Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
19 Mar 2015

This removes the check for the JForm object. If we don't use JForm in this class, we don't have to require it.

avatar Hackwar Hackwar - open - 19 Mar 2015
avatar joomla-cms-bot joomla-cms-bot - change - 19 Mar 2015
Labels Added: ?
avatar wilsonge
wilsonge - comment - 19 Mar 2015

For the unit tests we should use the @expectedException tag and split the tests up. So we have one test for each exception etc. It's more maintainable long term (I know it's not how we're doing it in a lot of places - but that's why those tests are so hard to maintain)

avatar rdeutz
rdeutz - comment - 19 Mar 2015

Agreed on removing the JForm object check.

One rule for testing is that each test should test one thing, a good sign of breaking this rule is when you have more then one assertion in a test. As any rule there are exceptions but in this case you should write a test for each Exception. The object under test should be created in the setup method, this makes sure that each test runs under clean conditions and test two is not effected from the result of test one. You should also mock the input object because this test shouldn't test if JRegistry works. Same might be true for element but this is discussable. I would mock it either because we are not testing if SimpleXMLElement is working.

avatar rdeutz
rdeutz - comment - 19 Mar 2015

Forget the mention, we have some documentation about writing tests: https://docs.joomla.org/Unit_Testing_Best_Practices

avatar brianteeman brianteeman - change - 19 Mar 2015
Labels Added: ?
avatar Hackwar
Hackwar - comment - 22 Mar 2015

I updated the PR.

I have to say that I disagree with the rule "one assert per method". I test one method of a class per method of a unittest.

avatar rdeutz
rdeutz - comment - 22 Mar 2015

I have to say that I disagree with the rule "one assert per method". I test one method of a class per method of a unittest.

Then you are testing only if a method works or not and not what the conditions of the failure are. If you check what the experts on this area say then you will find that this rule is widely accepted.

avatar wilsonge wilsonge - change - 22 Mar 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-03-22 15:53:57
avatar wilsonge wilsonge - close - 22 Mar 2015
avatar wilsonge wilsonge - reference | - 22 Mar 15
avatar wilsonge wilsonge - merge - 22 Mar 2015
avatar wilsonge wilsonge - close - 22 Mar 2015
avatar wilsonge
wilsonge - comment - 22 Mar 2015

That looks much better! Thanks Hannes :)

avatar rdeutz rdeutz - change - 22 Mar 2015
Milestone Added:
avatar Hackwar Hackwar - head_ref_deleted - 6 Jan 2016

Add a Comment

Login with GitHub to post a comment