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

Starting test suite for Craft #3382

Merged
merged 181 commits into from
Apr 20, 2019
Merged

Starting test suite for Craft #3382

merged 181 commits into from
Apr 20, 2019

Conversation

gtettelaar
Copy link
Contributor

@gtettelaar gtettelaar commented Oct 15, 2018

As discussed over Slack with @angrybrad i have been working on several tests for the helpers and validators for Craft. These tests are relatively low hanging fruit and they aren't many in numbers but its a start atleast.

Conventions followed

Followed the Yii 2 convention of namespacing the tests with craftunit\

Mock implementations of classes are found under _support/mockclasses. These classes are used to test method such as craft\helpers\Db::prepareValueForDb() where we need to test what happens if a serializable is passed in.

DB tests for the helpers are found under helpers/dbhelper/* Mysql or PostgreSql specific tests have their own specific test suite that gets skipped if the DB env is not applicable. This is done because for some functions inside this helper can result in different outputs with the same input I.E Running craft\helpers\Db::parseParam on 'grez can return different results than MySql in certain conditions.

One or two problems

On some tests we are getting issues with coverage simply because getting \Craft::$app->getRequest()->getIsCpRequest() to return true has proven difficult.

Also some Db helper tests are missing because i first want to figure out a different way to setup the test environment. Currently using the MySql dump works however it is painfully slow when running more tests. More importantly running tests with a Postgres .dump file is very buggy (atleast when i attempted it). Ill open up a separate issue to float the potential idea of setting up and breaking down the test environment with the install.php migration as this should be far more performant and should work equally on MySql and 'grez - aswell as provide a test for this specific code aswell which is an added bonus.

From here

Ill start working on finalizing the final tests for the above suite(a couple assertions are still missing) and move on to the Element and Migration helper. I want to get the helpers and validators done first and then start moving into the services.

Ill also start working on a Craft base plugin class which should also provide some support services for plugin/module testing and to see how we can setup fixtures for Craft's element types. In general i want see how we can integrate the work done by Bob Olde Hampsink/Robuust over at: https://github.com/robuust/craft-fixtures into the core.

Anyway. Thats my ramble done 👍. If theres anything that needs fixing ill be happy to hear it!

Added a base class test(future)
Added a json tester class and method
Added more date time testing methods
Added more app testing methods
Added 3 new tests to the date time helper.

Added db helper test
Added html helper test
Removed date.timezone from config tests

Added emoji filtering to the filterByValue
Added a mock of the component helper
Fixed bugs with the datetimehelper abstraction and a value misread
Fixed a typo in the db helper
added a second brace to the html helper mocks
Corrected an assumption for the json test
Added a mock for the urlhelper
Added assetManagerConfig to config checker.
Refactored the exception check in string test
Moved view test to under the /web dir
Added namespace and license to view test

Added correct license to all test classes
Some 'stubbing' is done to avoid working directly with Craft::$app->getSession(); Sessions in php CLI can be strange and it serves no added purpose to test this. Plus its nice to have an example in the codebase on a way to mock/stub classes.
: One of the tests might fail under very specific conditions. Added a note to ensure this gets fixed later
@gtettelaar
Copy link
Contributor Author

@narration-sd Thanks! Appreciate it!

I wanted to post an update on the test suite's current position. I managed to make more progress over the holidays on learning more about testing and adding more tests on the web/ and helpers/ classes. There are one or two things worth noting about the current direction of the tests as well as plans for this year.

Testing philosophy

It must be noted that the tests aren't strict 'unit' tests in the traditional sense. Strict unit testing commonly means that a lot of mocking must be done to isolate the function/block of code you are trying to test. This has several upsides however it does limit the scope of the test. The schematic plugin implements this style of testing in a really good way. See https://github.com/nerds-and-company/schematic/blob/master/tests/_support/Helper/Unit.php and you will see that the Craft::$app object is fully mocked.

Rather than mocking all parts of the Craft service locator, I opted for a more integration focused approach. In the current test suite the Craft::$app object is set up as normal (thanks to the Yii2 module) and there are many tests where we don't mock results of calls to the Craft::$app object as well as allowing interaction between the database e.t.c. This leads to a unit test for a single function (potentially) depending on results from external actors (the database, the Craft::$app object e.t.c.). Commonly this can be referred to as an integration style of testing. An upside of this is that you focus more on how craft components integrate and work together, the downside is obviously that tests can fail because of factors that have nothing to do with the function you are testing. This isn't to say that the PR currently doesn't implement mocking. Mocking is still done to ensure consistent results on dependencies that don't work well in a test environment or aren't required to test (Yii2 based functionality is a good example of this). As a practical example, we currently mock the session based functions in the craft\web\User as this is mostly Yii2 based and PHP sessions on CLI can be buggy.

An awsome comparison of the difference between these testing styles can be found here: https://martinfowler.com/articles/mocksArentStubs.html

Craft module:

This week I will be adding a commit that ensures the Craft codeception module can do the following things:

  • Setting up various plugins and module instances and their service locator (meaning the following code MyModule::getInstance()->myComponent->myFunction()) will be fully functioning. This means plugin and module devs will be able to perform unit, functional and acceptance tests where modules play a role.
  • Apply custom migrations to allow custom database schema to be present in tests
  • Add support to test event-based code. An expectEvent function is present that allows plugin devs to test their code's event firing
  • Setup the database according to the latest install migration. Meaning that the correct and updated schema will always be available. SQL dump files are no longer required.
  • Support functions for mocking

All of this will be configurable via the codeception.yml file. The module extends the Yii2 module so no functionality is lost.

Conclusion:

@angrybrad The testing philosophy is something I want to get your opinion on. Have there been any internal discussions at P&T on whether to use a more mocking focused style of testing versus a more integration focused style? If a more mocking style is preffered (i.e the schematic plugin) it wont be that much work to rewrite the tests to reflect this.

Timeline wise after the Craft module is complete I want to return to complete the web/ classes and then start moving on to the services. Ill start getting to this next week. Based on P&T's own statement of intent (https://craftcms.com/blog/state-of-craft-2018)
plus the work I want to put into adding more tests it should be possible to achieve the 80% threshold coverage marker by the next .all in Montreal (~August/September).

@angrybrad angrybrad changed the base branch from develop to feature/tests April 20, 2019 23:13
@lukeholder lukeholder merged commit a2f55a3 into craftcms:feature/tests Apr 20, 2019
@angrybrad
Copy link
Member

@lemiwinkz Thanks for your patience and the awesome work, Giel. We've spent the afternoon wrapping our head around the PR and think it's a great starting point. Have merged this into a branch here (https://github.com/craftcms/cms/tree/feature/tests) and will be working on it from there before merging into develop.

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