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

Test multiple database / storage adapters using Behat #550

Merged
merged 26 commits into from Jul 28, 2017

Conversation

Projects
None yet
2 participants
@christeredvartsen
Member

christeredvartsen commented Jun 23, 2017

This PR enables testing of all the available database and storage adapters in Imbo by adding suites to behat.yml.dist. Each suite must have an accompanying feature context class that must implement the ImboFeatureContext interface.

This currently only allows testing of the different database (Imbo\Database) and storage (Imbo\Storage) adapters. It should be expanded to also enable testing of other configurable adapters:

  • Imbo\Auth\AccessControl\Adapter
  • Imbo\EventListener\ImageVariations\Database
  • Imbo\EventListener\ImageVariations\Storage

christeredvartsen added some commits Jun 23, 2017

Enable autoloading of Behat context classes
Instead of adding the path of the Behat feature context classes to the
Behat configuration file, and manually including the files in the
bootstrap for the unit tests the directory is added to autoload-dev in
composer.json
Add interface for Behat feature context classes
The interface must be implemented by all feature context classes so they
can provide an instance of a database and storage adapter to be used in
the different suites for Behat.
Use PSR-4 instead of PSR-0 for context classes
To make the name of the Behat context feature classes more clear, we
should use PSR-4 instead of PSR-0. This allows for the usage of
underscores in the classname without having the autoloader automatically
assuming the _ in the classname matches a / in the path.

Because of this we can now match:

Database_Storage => %path%/Database_Storage.php

and not

Database_Storage => %path%/Database/Storage.php
Enable Behat testing of all adapters
This commit introduces different suites in the Behat test suite, which
enables testing of all the different database and storage adapters, and
combinations of these.

Currently only the MongoDB database adapter and the GridFS storage
adapter is tested, which has been the case since Imbo first started
using Behat for integration tests.

Each suite, which is a combination of a database adapter and a storage
adapter should be added to behat.yml.dist, and should follow the
following naming convention:

<database adapter>_<storage adapter>, for instance "mongodb_gridfs" and
must refer to a context class which follows the same convention, only
that the names should match the specific names of the adapters:

mongodb_gridfs => MongoDB_GridFS which in turn refers to the following
adapters:

- Imbo\Database\MongoDB
- Imbo\Storage\GridFS

Each new context class must implement the ImboFeatureContext interface,
which introduces the two methods that is required to return the database
and storage adapters. Each feature context can also have one or more
Behat hooks that can be used to setup / tear down the specific suites.
Each suite is expected to clear up all resources it generates in the
test run.

Resolves #279
@christeredvartsen

This comment has been minimized.

Show comment
Hide comment
@christeredvartsen

christeredvartsen Jun 23, 2017

Member

Before merging this PR the following adapters must be included in the tests:

  • Imbo\Database\Doctrine (MySQL) (e6b12bf)
  • Imbo\Database\Doctrine (SQLite) (134d8ec)
  • Imbo\Database\Doctrine (Postgres)
  • Imbo\Database\MongoDB (9bdd216)
  • Imbo\Storage\Filesystem (a3ec2c6)
  • Imbo\Storage\S3 (*)
  • Imbo\Storage\B2 (*)
  • Imbo\Storage\GridFS (9bdd216)

*) These might be somewhat problematic to test on Travis. If we are to test them we need to use private environment variables on Travis, and the test-runs might actually end up costing $'s.

Member

christeredvartsen commented Jun 23, 2017

Before merging this PR the following adapters must be included in the tests:

  • Imbo\Database\Doctrine (MySQL) (e6b12bf)
  • Imbo\Database\Doctrine (SQLite) (134d8ec)
  • Imbo\Database\Doctrine (Postgres)
  • Imbo\Database\MongoDB (9bdd216)
  • Imbo\Storage\Filesystem (a3ec2c6)
  • Imbo\Storage\S3 (*)
  • Imbo\Storage\B2 (*)
  • Imbo\Storage\GridFS (9bdd216)

*) These might be somewhat problematic to test on Travis. If we are to test them we need to use private environment variables on Travis, and the test-runs might actually end up costing $'s.

christeredvartsen added some commits Jun 24, 2017

Add a namespace for Behat feature context classes
This commits adds ImboBehatFeatureContext as a namespace to all classes
related to the feature context classes used in Behat tests. Some classes
have also been renamed to better suit their purpose.
Add traits for database and storage adapters
To more easily test combinations of database and storage adapters in the
Behat test suites we can use traits for the different adapters. Each
adapter should be responsible of setup / teardown functionality related
to the different adapters. Each suite could then simply use two traits
in combination to test them.
Skip traits and use configuration for suites
Instead of having traits for the different adapters we want to test, and
a class for each combination this commit will use configuration from the
suites in the behat.yml[.dist] file instead. We also need to exchange
configuration between the context triggered by Behat (which runs ::setUp() /
::tearDown()), and the context from when Imbo gets a request from the test
(::getAdapter). This is done with a request header. The main feature
context has been renamed back to FeatureContext as we no longer need the
previously added FeatureContext interface. The interface added two
methods that is no longer needed.
Provide sane default values for the Images model
This gives the Imbo\Model\Images model sane deafult values for hits,
limit and page.

Resolves #553
Add test-suite for Doctrine/SQLite and filesystem
Adds a separate suite for testing the combination of the
Imbo\Database\Doctrine database adapter (with a SQLite database) and the
Imbo\Storage\Filesystem storage adapter.
Add database setup for Doctrine/SQLite test
The doctrine_sqlite_filesystem suite uses a SQLite database for testing,
and this class is responsible for setting up / clearing the database
used for testing.
Adjust comparison to work for non-type databases
As the Doctrine database does not handle types in the same way as MySQL
/ MongoDB we need to adjust the tests as the Behat API Extension is
type-sensitive on comparisons such as theses.
Order by the update flag for correct results
To get the lastest value from the updated column the adapter must add
ordering.

Resolves #556.
Add method for fetching users in the database
This is a helper method for fetching the unique user names currently
found in the database. If no users-filter is used with the global images
endpoint we need to fetch the list of all users currently in the
database to be sure that the public key has access to all of them.
Clean up tests for database adapters
Some missing tests have been added, and a generic clean-up has been done
as well, as will be done to all test cases. Add test for the newly added
getAllUsers-method.
Fix getImages and getLastModified
Implement the new behaviour for getImages, where images from all users
will be returned if the users-parameter is empty. The
getLastModified-methods are also updated and implements the behaviour as
specified in the method documentation found in the interface.
Remove is_array check
The Request::getUsers() method always returns an array, so no need for
the check.
Fetch all users for auth when query param is empty
When the client does not specify the users-filter when requesting the
global images endpoint Imbo should use all users found in the current
database to use for authentication. Also added test case to verify new
behaviour.
Use setup file for SQL
Instead of duplicating the SQL for creating the tables in the database
the test classes now use the contents of the file found in the setup dir,
which is also included in the documentation.
Supply suite settings for the setUp method
Some suites needs extra settings that can be used in the set up, so this
commit adds a parameter to the setUp method that holds all settings for
the  current suite.
Add suite for Doctrine (MySQL) and Filesystem
This commit adds a Behat suite for testing the Doctrine adapter with a
MySQL database in combination with the Filesystem storage adapter. For
this to work we need to enable the mysql service in Travis-CI.
@christeredvartsen

This comment has been minimized.

Show comment
Hide comment
@christeredvartsen

christeredvartsen Jul 28, 2017

Member

@matslindh Care do to a quick review so we can get this merged? I'm not including suites for Postgres (Doctrine), S3 or B2 for now. These can be added later.

Member

christeredvartsen commented Jul 28, 2017

@matslindh Care do to a quick review so we can get this merged? I'm not including suites for Postgres (Doctrine), S3 or B2 for now. These can be added later.

@matslindh

I'm not familiar with the Behat stuff, so I've just given it a very rudimentary read. A few stylistic changes otherwise. Also surprised that we don't have visibility qualifiers (public) in the interface definition?

if ($imageIdentifier) {
if ($imageIdentifier !== null) {
$query->andWhere('i.imageIdentifier = :imageIdentifier')
->setParameter(':imageIdentifier', $imageIdentifier);
}
$stmt = $query->execute();

This comment has been minimized.

@matslindh

matslindh Jul 28, 2017

Contributor

We should probably have something similar to LIMIT 1; here, so that the DB doesn't have to build the complete result set, just find the one sorted at the top? Or SELECT MAX(...)..

@matslindh

matslindh Jul 28, 2017

Contributor

We should probably have something similar to LIMIT 1; here, so that the DB doesn't have to build the complete result set, just find the one sorted at the top? Or SELECT MAX(...)..

Show outdated Hide outdated src/Database/Doctrine.php
Show outdated Hide outdated src/Database/MongoDB.php
Show outdated Hide outdated src/Database/Doctrine.php
Show outdated Hide outdated src/Database/MongoDB.php
Show outdated Hide outdated tests/behat/features/bootstrap/DatabaseTest/DoctrineMySQL.php
Show outdated Hide outdated tests/behat/features/bootstrap/DatabaseTest/DoctrineSQLite.php
Show outdated Hide outdated tests/phpunit/integration/Database/DoctrineTest.php
Fix issues raised in code review of #550
As we can't define constants through the Behat configuration file I
decided to simply add a config entry in each suite for the path to the
project root. The suites don't inherit configuration from some default
place, so the same key => value has to be defined in every suite. For
PHPUnit a project root constant has been set in the bootstrap script.
@christeredvartsen

This comment has been minimized.

Show comment
Hide comment
@christeredvartsen

christeredvartsen Jul 28, 2017

Member

All issues have been fixed in 3e024c0.

With regards to the missing public-keyword from the interfaces, this is basically just something I started doing years back, as all methods in the interfaces are public anyways. I'm not really against having them there, it's just a habit of mine to not include them.

Member

christeredvartsen commented Jul 28, 2017

All issues have been fixed in 3e024c0.

With regards to the missing public-keyword from the interfaces, this is basically just something I started doing years back, as all methods in the interfaces are public anyways. I'm not really against having them there, it's just a habit of mine to not include them.

@christeredvartsen christeredvartsen merged commit c8f2103 into develop Jul 28, 2017

1 check passed

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

@christeredvartsen christeredvartsen deleted the issue/279-test-multiple-adapters branch Jul 28, 2017

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