ISSUE_231

[ioigoume]

#231

[codecov]

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 53.04%. Comparing base (9eb64b1) to head (dfe0a2e).
Report is 1 commits behind head on wip-version-6.

Files with missing lines	Patch %	Lines
src/Services/DatabaseMigration.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##             wip-version-6     #233      +/-   ##
===================================================
+ Coverage            52.84%   53.04%   +0.19%     
- Complexity            1155     1156       +1     
===================================================
  Files                  122      122              
  Lines                 4320     4323       +3     
===================================================
+ Hits                  2283     2293      +10     
+ Misses                2037     2030       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ioigoume]

For the PostgreSQL database, the tests are failing because a boolean is not a tinyint as for the case of MySQL or SQLite.

[pradtke]

Thanks for the fix @ioigoume , and for expanding the revocation tests to cover postgresql and mysql.

One issue I ran into is that on my Mac the containers can't be accessed by IP but must be accessed by port mapping from localhost. E.g. I had to do something like

$hostPort = '15432';
$pgContainer->withPort($hostPort, '5432');
...
'database.dsn' => sprintf('pgsql:host=%s;port=%s;dbname=database',  'localhost', $hostPort),

I assume you are using Linux (and that is what the unit tests are running on in the github actions). Does doing port mapping and connecting to localhost work on Linux? I'm wondering what is a cross platform way we can address connecting to the containers.

A secondary thing is the conformance-suite github action step is failing on docker-compose not be available. I don't know if that is just a transient thing (since it worked in your earlier test runs), or if there is some adjustment that needs to be made.

[pradtke]

I got the docker based DB tests to run on Mac, and I think I kept the previous Linux behavior.
@cicnavi when you have a moment, can you confirm the RevokeTokenByAuthCodeIdTraitTest tests pass for you? I want to confirm there is not other oddities with connecting to docker containers. The test should run a postgres and mysql container and confirm the db migrations and revocation work.

[cicnavi]

Hi, hm, I use docker containers as my dev environment (I don't develop locally...). So I get error that the command 'docker' is not available (in my dev container)...

impleSAML\Test\Module\oidc\Repositories\Traits\RevokeTokenByAuthCodeIdTraitTest
Symfony\Component\Process\Exception\ProcessFailedException: The command "'docker' 'run' '--rm' '--detach' '--name' 'testcontainer66cc36d32c16f0.24369224' '-p' '5432:5432' '--env' 'POSTGRES_PASSWORD=password' '--env' 'POSTGRES_DB=database' '--env' 'POSTGRES_USER=username' 'postgres:15.0'" failed.

Exit Code: 127(Command not found)

Working directory: /var/www/projects/oidc/simplesamlphp-module-oidc

Output:
================


Error Output:
================


/var/www/projects/oidc/simplesamlphp-module-oidc/vendor/symfony/process/Process.php:269
/var/www/projects/oidc/simplesamlphp-module-oidc/vendor/testcontainers/testcontainers/src/Container/Container.php:181
/var/www/projects/oidc/simplesamlphp-module-oidc/tests/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php:148
/var/www/projects/oidc/simplesamlphp-module-oidc/tests/src/Repositories/Traits/RevokeTokenByAuthCodeIdTraitTest.php:92

[tvdijen]

@cicnavi This may have to do with my last commit to master where I fixed the conformance tests in GHA. The container you're using probably still expects docker-composer.

Also see https://github.com/orgs/community/discussions/116610

[cicnavi]

I'm aware of docker compose change and have seen your commits. I can successfully run it, it's not that...

[cicnavi]

And... now it also crossed my mind that you are actually doing integration testing (compatibility with dependent services, different parts / different systems...). This doesn't actually belong to unit tests, which should be small parts of code ran in isolation (with mocks to external dependencies if necessary)....

Don't get me wrong, I think what you guys did is great and valuable, but it would be great if you could do it 'outside' of unit tests...

[pradtke]

@cicnavi Ah, so you are already in docker container when phpunit runs and therefor can't launch additional containers.

I get what you are saying about these being integration tests. I think the other issue is that we've had 3 developers attempt to run the tests against MySQL and Postgres and the 3 of us have all had different issues getting them to work with Docker.

🤔 @ioigoume and I can chat more about it this week.

Would you find it useful to run DB integration tests locally? Or would you just rely on github actions to do it? It will help us think through how to approach this.

[cicnavi]

I think there is nothing wrong with running it locally (and in GHA). I would just like to have it separate from current unit tests so we can continue running them quickly (they currently take less then 10 secs), with standard vendor/bin/phpunit command, without the requirement of having docker installed....

There is actually a sample in phpunit docs which separates unit and integration tests: https://docs.phpunit.de/en/10.5/organizing-tests.html

Or maybe having a totally separate phpunit config for "DB compatibility tests", since for them you would have a special instruction that they are dependent on having docker installed or similar...

[cicnavi]

I've tried running unit tests locally and it works fine, thanks!

[pradtke]

Thank you @ioigoume for improving our automatic DB validation