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

[WIP] Add tests #185

Merged
merged 9 commits into from
Feb 18, 2018
Merged

[WIP] Add tests #185

merged 9 commits into from
Feb 18, 2018

Conversation

Stoakes
Copy link
Member

@Stoakes Stoakes commented Nov 2, 2017

This is a work in progress.

@Stoakes Stoakes force-pushed the add-tests branch 7 times, most recently from e01571d to 3e0fbc0 Compare November 2, 2017 19:29
- docker-compose up -d
# - docker-compose run --rm web composer install -o -n
- sleep 10 && docker-compose run --rm web php bin/console cache:clear -e=test --no-warmup
- sleep 10 && docker-compose run --rm web php bin/console cache:clear -e=prod --no-warmup

Choose a reason for hiding this comment

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

is docker necessary here? You have a VM you can freely configure, docker is a major slowdown

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is.
We rely on Docker to provide a 5 command line installation. It brought several Junior-Entreprise on board thanks to that feature. Travis checks that the install process is working for every commit.

@@ -85,6 +85,12 @@ public function createDatabase()
]);
$output = new BufferedOutput();
$application->run($input, $output);

$input = new ArrayInput([

Choose a reason for hiding this comment

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

Don't create a database for your tests before each scenario, it's horribly slow. Instead create a clean empty database before running your tests, have a @BeforeScenario hook starting a transaction and rollback that transaction in @AfterScenario. It is way more efficient, the only downside would be if you wanted to take a peek at the database content in the middle of the tests which wouldn't work anymore.

Note that it won't work with SQLite, but I recommend to not use SQLite anyway as it doesn't have real support of any of the RDBSM features of PostgreSQL or MySQL such as transactions, foreign key constraints, cascade persist/delete etc.

@@ -85,6 +85,12 @@ public function createDatabase()
]);
$output = new BufferedOutput();
$application->run($input, $output);

$input = new ArrayInput([
'command' => 'demo:create_data',

Choose a reason for hiding this comment

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

I also recommend to avoid to load global data for your tests and instead load specific data you need for your scenarios. It's way easier to manage. If you change the data loaded globally, it can very easily break a lot of tests that were depending on it, if your data is specific for each scenario, it can only break that specific scenario

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true.

However, we have too few developers to afford such additional work right now.

# the "@createSchema" annotation provided by Behat creates a temporary SQLite database for testing the API
@createSchema
Scenario: I can export the default AP
Given I am logged in as "admin"

Choose a reason for hiding this comment

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

a scenario should be "Given, When and Then". Given a predicate, when I do an action, then I get this reaction.

Choose a reason for hiding this comment

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

Given I am logged as admin
And I have an etude "Etude 2"
When I access the etude "Etude 2"
Then I should see the etude "Etude 2"

Some comments:

  • When creating objects, you can give them a label like "Etude 2" above, so in a context you can have an array label => object to easily access to them.

  • It seems bad phrasing (and it is), the label is used again and again at each sentence. While it could be phrased better, this approach have a big advantage: it makes it stateless. If you don't want to repeat yourself, you would need to keep track of the subject which is a PITA and very error prone. So trading off on the phrasing a bit is a good idea here

  • And I have an etude "Etude 2": so you create your object and you store it with the label Etude 2

  • When I access the etude "Etude 2": for an UI tests, it can be "visit the page X". For a non-UI tests, it can be get the object from the repository

  • Then I should see the etude "Etude 2" for your UI tests, you can indeed test the response type (HTML, JSON...) and the status code

# The "@dropSchema" annotation must be added on the last scenario of the feature file to drop the temporary SQLite database
@dropSchema
Scenario: I can delete a Filiere
Given I am logged in as "admin"
Given I am on "/personne/filiere/modifier/1"
Given I am on "/personne/filiere/modifier/6"

Choose a reason for hiding this comment

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

You're using MySQL which is a bit of a shame, but if you ever switch to PostgreSQL (which is better in every imaginable ways), the auto-generation of ids is made by sequences. For example for user, if you have auto-generated int IDs for it, it would be provided by a sequence user_id. All the sequence does is creating a new value and incrementing itself. While this seem a bit more complicated, it allows you to easily reset it between each scenarios, which means you have a better control on the IDs used

Copy link
Member Author

@Stoakes Stoakes Nov 5, 2017

Choose a reason for hiding this comment

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

Ok.

By the way, MySQL is suited for the Jeyser CRM project.

Moreover we will not switch because:

  • It would break every already running installation (or create a cumbersome migration for little to no additionnal business value)
  • It makes imports easier because Siaje also uses MySQL.
  • Tooling is better (pgadminTools << PhpMyAdmin)
  • It would break all the previous migrations

Copy link

@theofidry theofidry Nov 5, 2017

Choose a reason for hiding this comment

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

If you're using InnoDB it should be possible to reset the auto-increment with:

ALTER TABLE tablename AUTO_INCREMENT = 1

which would be an alternative solution.

Switching of DB is never easy so I don't recommend to invest too much time on it. It might however be worth checking if the migrations have any MySQL specific features or not. It's far easier to switch from MySQL to PostgreSQL than vice-versa as MySQL is much more limited than Postgres.

Regarding the tooling I don't have much of an opinion as I'm really fine using the CLI client, it's something you need do at some point anyway but I understand phpmyadmin can be enough for limited usage.

Demo data are now quite complete, and are valid. Thus they are relevant for functionnal and behavioural testing

Also, change @afterstep handler to display page url, page title and beginning of content if the step fails.
@Stoakes Stoakes force-pushed the add-tests branch 10 times, most recently from cff1c23 to cee0b99 Compare February 18, 2018 11:51
Everything is green :)
@Stoakes Stoakes merged commit 7f71439 into master Feb 18, 2018
@Stoakes Stoakes deleted the add-tests branch February 21, 2018 14:08
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.

2 participants