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

Unittests for Notequals rule #6504

Merged
merged 3 commits into from Mar 22, 2015

Conversation

Projects
None yet
5 participants
@Hackwar
Member

Hackwar commented Mar 19, 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.

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Mar 19, 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)

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Mar 19, 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.

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Mar 19, 2015

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

@Hackwar

This comment has been minimized.

Member

Hackwar commented Mar 22, 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.

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Mar 22, 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.

wilsonge added a commit that referenced this pull request Mar 22, 2015

Merge pull request #6504 from Hackwar/patch-48
Unittests for Notequals rule

@wilsonge wilsonge merged commit 8f2e282 into joomla:staging Mar 22, 2015

1 check passed

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

This comment has been minimized.

Contributor

wilsonge commented Mar 22, 2015

That looks much better! Thanks Hannes :)

@rdeutz rdeutz added this to the Joomla! 3.4.2 milestone Mar 22, 2015

@Hackwar Hackwar deleted the Hackwar:patch-48 branch Jan 6, 2016

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