Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add some new tests, clean up some others. #1736

Merged
merged 1 commit into from Dec 16, 2012

Conversation

Projects
None yet
2 participants
Contributor

elinw commented Dec 3, 2012

This adds tests to a number of classes and also reorganizes some to be more useful (for example separating into separate tests so all assertions are tested regardless of failures of other assertions) including JFactory, JApplicationBase, JSession, and JUserHelper.

Contributor

elinw commented Dec 5, 2012

There is one failing test on this pr which is the getExpire() test. The fact is the method works as expected but the expected value should be 900. If you dump out the object that is what is there, and that is what getExpire() returns. The problem is that the test as written has expected value of 20 which is what you would expect if $this->object had actually been successfully defined at the top of the test class. However, when the full suite of tests is run this does not happen and instead the expiration is set to 900 which is the default of 15 * 60 as calculated in the factory. Because of how sessions are managed changing this is complicated and it seems unnecessary since all we want to know is if the method is returning the value that the property has. So therefore I am thinking that I will change the expected to 900 and have all passing tests. (When just some tests are run the test currently passes with a value of 20. We may or may not consider it a problem that the value is not changing to 20 but that is not the responsibility of this specific test.)

Yes we do have a default session length of 15 hours (see #1715) but that's not related to whether getExpire() works.

Contributor

elinw commented Dec 6, 2012

Just to be clear I don't actually think this is ready to merge because the fact is that there is something wrong with how this whole test is working, which may be a problem in the test or may be a problem in the class. It should not be the case that the expecteds change depending on whether you run the test by itself or if you run it as part of a full suite or if you add a new test to the suite.
The getInstance test actually illustrates exactly that you can't change the expired value using getInstance when there is an existing session just by using getInstance. Dumping out the oldSession and newSession objects shows that they are both the same object and both have expire=900 despite what is set in the test.
If instead we started the test with
$this->object = new JSession('none', array('expire' => 20, 'force_ssl' => true, 'name' => 'name', 'id' => 'id', 'security' => 'security'));
we would see that the getInstance tests fail and we would also see the getFormTokenTest fail but the old getExpire test passes with expected of 20.

I'm reasonably certain that the getInstance test should be
$this->assertSame(
$oldSession,$newSession
);

but either way it is the same thing in terms of the test passing because it's just getting a default session i.e. expire=900 for both in the current test.

Contributor

elinw commented Dec 8, 2012

Ok now if jenkins says it's ok I think these are ready.

@pasamio pasamio added a commit that referenced this pull request Dec 16, 2012

@pasamio pasamio Merge pull request #1736 from elinw/usertests
Add some new tests, clean up some others.
e6a697e

@pasamio pasamio merged commit e6a697e into joomla:staging Dec 16, 2012

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