Skip to content
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

[Tests] Errors with prepareTestedSiblings() lead to non execution of tests #49

Closed
vjandrea opened this issue Oct 6, 2013 · 21 comments
Closed
Labels

Comments

@vjandrea
Copy link
Contributor

vjandrea commented Oct 6, 2013

There's something wrong with the return value of prepareTestedSiblings() because everytime it's called i get a InvalidArgumentException: Value must be provided. error message.
I have an issue with xdebug and i can't dig deeper into this at the moment, anyway i noticed that if there's an error a test execution is halted, because a $this->assertEquals(true, false) dropped below the assignment is never evaluated.

@franzose
Copy link
Owner

franzose commented Oct 6, 2013

Strange, I use phpunit, as laravel says, and no errors appear.

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 6, 2013

I have two ClosureTable folders, one inside a laravel project that i update via composer to your repo, one in my documents folder, and in both cases i get the same errors.
htdocs: http://paste.laravel.com/WRv
documents: http://paste.laravel.com/WRw

OSX 10.7.5 / macbookpro
PHP version: 5.4.10 (MAMP)
PHPunit version: 3.7.27

Dependencies installed through composer (identical in both folders)

  illuminate/container 4.0.x-dev ee86c89 
  illuminate/database  4.0.x-dev 64c9e48 
  illuminate/events    4.0.x-dev 2253c8c 
  illuminate/support   4.0.x-dev bec895e 
  nesbot/carbon        1.4.0             A simple API extension for DateTime.

If you need more info let me know!

@vjandrea vjandrea closed this as completed Oct 6, 2013
@vjandrea vjandrea reopened this Oct 6, 2013
@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 6, 2013

Updated php to 5.5.3, errors persist. I don't know where i should bang my head on. I'm going to setup a vm with vagrant to test on linux.

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 6, 2013

I did further tests on a vm created with vagrant from this repo: https://github.com/bryannielsen/Laravel4-Vagrant and i still get all 34 errors: http://paste.laravel.com/WV4

@franzose
Copy link
Owner

franzose commented Oct 6, 2013

My setup is Mac OS X 10.7.5 and I don't use virtual machine. PHP is:

PHP 5.4.16 (cli) (built: Jun 15 2013 04:04:59) 
Copyright (c) 1997-2013 The PHP Group
Zend Engine v2.4.0, Copyright (c) 1998-2013 Zend Technologies
    with Xdebug v2.2.2, Copyright (c) 2002-2013, by Derick Rethans

PHPUnit 3.7.21 by Sebastian Bergmann.

@franzose
Copy link
Owner

franzose commented Oct 7, 2013

Line 544 is where $this->getKey() is called, hm...

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 7, 2013

What makes me think is that before the last call to $this->prepareTestedSiblings(), it stops on line 70, at the end of this:

$child = $page->appendChild(Page::make(array(
            'title' => 'Child Page Test Title',
            'excerpt' => 'Child Page Test Excerpt',
            'content' => 'Child Page Test content',
            'language' => 'en'
        )), 0, true);

or on line 117, at the end of this:

$child1 = $page->appendChild(Page::make(array(
            'title' => 'Test Title',
            'excerpt' => 'Test Excerpt',
            'content' => 'Test content',
            'language' => 'en'
        )), 0, true);

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 7, 2013

Tried on Windows 7 32bit (MINGW32_NT-6.1) as well, PHP 5.5.4 (cli), PHPUnit 3.7.27, i still get all 34 errors:
http://paste.laravel.com/X44

@franzose
Copy link
Owner

franzose commented Oct 8, 2013

How have you sped, Andea?

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 8, 2013

@franzose, nothing new at the moment, i tried to write other tests for prepareTestedSiblings() but i always get the InvalidArgumentException. I am now studying a bit phpunit API to see if i can make it spit out a more detailed log, because 'Value must be provided' is not really helpful as hint. By the way, did you manage to see the errors in your phpunit?

@franzose
Copy link
Owner

franzose commented Oct 8, 2013

@vjandrea, and now, you wouldn't believe, I've got the same 34 errors! great. So, now it's a problem of both of us :D

@franzose
Copy link
Owner

franzose commented Oct 8, 2013

And I know the reason.

@franzose
Copy link
Owner

franzose commented Oct 8, 2013

@vjandrea, when we call Entity::make(), it tries to guess and set the position if not provided by this: $model->guessPositionOnCreate();. What do we see inside that method? Yes, we see $this->hasSiblings() and $this->lastSibling() calls. Exception is thrown right after $this->hasSiblings() call because... The query is built by buildSiblingsQuery() and buildSiblingsSubquery() methods. The latter requires id of the model. But the model doesn't have it yet because hasn't been saved into the database!

@franzose
Copy link
Owner

franzose commented Oct 8, 2013

We have to change somehow the way position is guessed.

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 8, 2013

Amazing! Now i know where i should bang my head on! I'm trying to figure out something.

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 8, 2013

So, what if we put a check in hasSiblings() that returns false if the id is not set? I think we should avoid the creation of the query in that case.

@franzose
Copy link
Owner

franzose commented Oct 8, 2013

@vjandrea, I think that can break things. Now I'm trying to assume that when we make(), we can set position to 0 as well as the depth. Part of errors are gone after that, but still there's 5.

@vjandrea
Copy link
Contributor Author

vjandrea commented Oct 9, 2013

@franzose that's great! Do you mind pasting the error dump of the remaining 5, or create a hotfix branch with your edit that i can pull to test?

@franzose
Copy link
Owner

@vjandrea, aaaaaaaaarrgghhhhhh! Sorry, but it's fucking great. I have solved problems with siblings at last! Sorry, but you'll need to update your repository and merge with your changes.

franzose added a commit that referenced this issue Oct 11, 2013
fixed #49, many problems with querying siblings
@vjandrea
Copy link
Contributor Author

Brilliant job, @franzose!

@franzose
Copy link
Owner

@vjandrea thanks, however I suppose that there's still problem with the position column. I will discover that a little later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants