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

Fix master branch and its travis tests #397

Merged
merged 3 commits into from
Jan 28, 2018
Merged

Fix master branch and its travis tests #397

merged 3 commits into from
Jan 28, 2018

Conversation

ElectricMaxxx
Copy link
Member

I just wanna try to fix the tests on master.

@ElectricMaxxx ElectricMaxxx changed the title change to trigger travis from master Fix master branch and its travis tests Nov 26, 2017
web/config.php Outdated
}

if (!in_array(@$_SERVER['REMOTE_ADDR'], [
if (!in_array(@$_SERVER['REMOTE_ADDR'], array(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My IDE does it all the way. i will remove it.

@@ -23,6 +23,7 @@ matrix:

env:
global:
- SYMFONY_PHPUNIT_DIR=.phpunit SYMFONY_PHPUNIT_REMOVE="symfony/yaml" SYMFONY_PHPUNIT_VERSION=5.7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allowed, all on one line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik that works, yes. probably because this is how environment variables are passed anyways to the scripts that are run - travis-ci likely just concats the rows with whitespace separator.

@dbu
Copy link
Member

dbu commented Nov 27, 2017

@nicolas-grekas would you have an idea why the phpunit-simple crashes the way it does on travis-ci with everything but php 7.1?

@nicolas-grekas
Copy link

No idea, maybe be a memory limit that travis has in place? Can you make it work locally on 7.0?

@ElectricMaxxx
Copy link
Member Author

The difference is not the php version. I the composer-instal flag we use in travis.

@ElectricMaxxx
Copy link
Member Author

but i'll try it at home tonight

@SenseException
Copy link

AdminTest::testAdmin() seems to have a lot of work. What's tested there?

@ElectricMaxxx
Copy link
Member Author

Yes, the test is a little bit longer, as it calls each admin page we have in the sandbox and checks wheter it returns a 200 or not.

@SenseException
Copy link

On skipping that test: Call to undefined method Doctrine\ODM\PHPCR\Event\PreUpdateEventArgs::getDocument().

@ElectricMaxxx
Copy link
Member Author

Interesting. Started to divide the test trough a dataprovider today, but did not get it working yet.

@SenseException
Copy link

I've also tested AdminTest isolated, ignoring the other ones, which makes the build go green. But the test triggers a warning.

There was 1 warning:

1) Warning
The data provider specified for Tests\Functional\AdminTest::testAdmin is invalid.
Data set #0 is invalid.

foreach ($admins as $admin) {
$this->doTestReachableAdminRoutes($admin);
}
return $admins;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$admins is an one dimensional array of AdminInterface which is not a valid dataprovider.

@SenseException
Copy link

SenseException commented Dec 1, 2017

@ElectricMaxxx Can you please try the following code for the getAdmin data provider in your AdminTest?

public function getAdmin()
{
    $pool = $this->getContainer()->get('sonata.admin.pool');
    $adminGroups = $pool->getAdminGroups();

    $admins = function() use ($adminGroups, $pool) {
        foreach (array_keys($adminGroups) as $adminName) {
	    yield $pool->getAdminsByGroup($adminName);
        }
    };

    return $admins();
}

At least to see if this will make the build run until the end.

@ElectricMaxxx
Copy link
Member Author

@SenseException you can do aa PR against my branch, pleaae? I am currently mobile only.

@SenseException
Copy link

I'll see if I can do it at late evening.

@SenseException
Copy link

@ElectricMaxxx I've created #398.

@ElectricMaxxx
Copy link
Member Author

This one should fix it: sonata-project/cache#103

@ElectricMaxxx
Copy link
Member Author

@dbu having a look at this "bug" is really wired. On the one hand caching should never have worked, as there was never a method call getObject(), but this does not matter. A thing that makes me crazy is: why do we trigger a pre update event when calling a admin route by a GET request? Is there something else wrong on our side?

@ElectricMaxxx
Copy link
Member Author

aannd ... the AutoRouteListener does a flush again (what looks strange, the check) and preUpdate is triggerd on DoctrinePHPCRODMListnerContainerAware There must be an issue on this path.

@dbu
Copy link
Member

dbu commented Dec 6, 2017

yeah, we should not be generating anything at that point. and i don't know why we would want sonata cache active by default, there is no active cache invalidation anywhere so it should not be listening in, imho. i wonder if thats a default that changed on sonata side somewhere.

@ElectricMaxxx
Copy link
Member Author

Seems as i come closer: https://travis-ci.org/symfony-cmf/cmf-sandbox/jobs/333956339
The issue is the php version in sonata-project/cache. So we can either have an issue with a broken listener in cache (<v1.1.1) or we can have an issue with php version ...
So i bumped php versions for now.

@ElectricMaxxx
Copy link
Member Author

yea travis became green. So there was one chance only: going on php 7.1 and drop lower.

@ElectricMaxxx
Copy link
Member Author

No clue what the problem of plattform is, the link seems to be fine

@ElectricMaxxx ElectricMaxxx merged commit d014fa8 into master Jan 28, 2018
@ElectricMaxxx ElectricMaxxx deleted the fix_master branch January 28, 2018 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants