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

Introduce modern unit tests for new test structure #11525

Merged
merged 5 commits into from Nov 29, 2018

Conversation

@matks
Copy link
Contributor

matks commented Nov 27, 2018

Questions Answers
Branch? develop
Description? Introduce new unit tests structure, setup, scripts
Type? refacto
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? #10733
How to test? Travis tests should be green and include the new unit test

This change is Reviewable

@@ -124,7 +124,8 @@
},
"autoload-dev": {
"psr-4": {
"Tests\\": "tests-legacy/"
"Tests\\": "tests-legacy/",
"TestsV2\\": "tests/"

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 27, 2018

Contributor

PsTests should be a proper name no?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 27, 2018

Contributor

How about LegacyTests ? :)

This comment has been minimized.

@matks

matks Nov 27, 2018

Contributor

Yes, I had a lack of inspiration 😛

This comment has been minimized.

@eternoendless

eternoendless Nov 27, 2018

Member

I vote for LegacyTests

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 27, 2018

Contributor

Yep LegacyTests 👍

@mickaelandrieu
Copy link
Contributor

mickaelandrieu left a comment

Only minor changes, we need to figure out a good name for namespace.

@@ -124,7 +124,8 @@
},
"autoload-dev": {
"psr-4": {
"Tests\\": "tests-legacy/"
"Tests\\": "tests-legacy/",
"TestsV2\\": "tests/"

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 27, 2018

Contributor

How about LegacyTests ? :)

UNIT=$?

if [[ "UNIT" == "0" ]]; then
echo -e "\e[92mPHPUNIT TESTS OK"

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 27, 2018

Contributor

PHPUNIT UNIT TESTS :p

This is PrestaShop automated tests folder.

Multiple type of tests are available:
- `E2E` and `Selenium` folders contain end-to-end tests maintained by PrestaShop QA team

This comment has been minimized.

@eternoendless

eternoendless Nov 27, 2018

Member

I don't think it's a good idea to say they are maintained by a specific group. Anyone should be able to add/improve E2E tests, starting with the Core team.

Suggested change Beta
- `E2E` and `Selenium` folders contain end-to-end tests maintained by PrestaShop QA team
- `E2E` and `Selenium` folders contain end-to-end tests
This folder contains legacy unit and integration tests written for PrestaShop 1.7.0 to 1.7.5.

As the structure of this folder evolved a lot in a short time, this resulted into multiple test methods being
used : unit tests with phpunit, unit tests with phpunit using PrestaShop Core classes or a database fixtures,

This comment has been minimized.

@eternoendless

eternoendless Nov 27, 2018

Member

Small punctuation problem.

Suggested change Beta
used : unit tests with phpunit, unit tests with phpunit using PrestaShop Core classes or a database fixtures,
used: unit tests with phpunit, unit tests with phpunit using PrestaShop Core classes or a database fixtures,

- one php class = one test file
- the test filepath must follow the class filepath
- every dependency of the class must be mocked*

This comment has been minimized.

@eternoendless

eternoendless Nov 27, 2018

Member

Watch out, "mock" may not the correct term. According to Martin Fowler, there are actually several kinds of test doubles:

Meszaros uses the term Test Double as the generic term for any kind of pretend object used in place of a real object for testing purposes. The name comes from the notion of a Stunt Double in movies. (One of his aims was to avoid using any name that was already widely used.) Meszaros then defined four particular kinds of double:

  • Dummy objects are passed around but never actually used. Usually they are just used to fill parameter lists.
  • Fake objects actually have working implementations, but usually take some shortcut which makes them not suitable for production (an in memory database is a good example).
  • Stubs provide canned answers to calls made during the test, usually not responding at all to anything outside what's programmed in for the test.
  • Spies are stubs that also record some information based on how they were called. One form of this might be an email service that records how many messages it was sent.
  • Mocks are what we are talking about here: objects pre-programmed with expectations which form a specification of the calls they are expected to receive.

In order to avoid misunderstandings, I suggest this change:

Suggested change Beta
- every dependency of the class must be mocked*
- every dependency of the class must be replaced by [test doubles][1]*
[1]: https://martinfowler.com/articles/mocksArentStubs.html#TheDifferenceBetweenMocksAndStubs
## Conventions
Use camelCase names for test function names.

This comment has been minimized.

@eternoendless

eternoendless Nov 27, 2018

Member
Suggested change Beta
Use camelCase names for test function names.
- Use camelCase names for test function names.
- Try to make method names explain the *intent* of the test case as best as possible. Don't hesitate to write long method names if necessary.
- Bad example: `testGetPrice` (no idea what such a test is supposed to do)
- Good example: `testDiscountIsAppliedToFinalPrice`
@@ -124,7 +124,8 @@
},
"autoload-dev": {
"psr-4": {
"Tests\\": "tests-legacy/"
"Tests\\": "tests-legacy/",
"TestsV2\\": "tests/"

This comment has been minimized.

@eternoendless

eternoendless Nov 27, 2018

Member

I vote for LegacyTests

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 27, 2018

Namespace will be migrated in #11529
This PR can be merged after.

@matks matks force-pushed the matks:new-unit-tests-1 branch from 193ba4d to 9b84470 Nov 28, 2018

Changes applied

exit 0;
else
echo -e "\e[91mUNIT TESTS FAILED"
exit 255;

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Nov 28, 2018

Member

Why don't you exit $UNIT ?

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 28, 2018

Contributor

👍

#!/bin/bash

composer run-script unit-tests --timeout=0;
UNIT=$?

if [[ "$UNIT" == "0" ]]; then
  echo -e "\e[UNIT TESTS OK"
else
  echo -e "\e[91mUNIT TESTS FAILED"
fi
exit $UNIT;

Multiple type of tests are available:
- `E2E` and `Selenium` folders contain end-to-end tests
- `Unit` folder contain unit tests, see below

This comment has been minimized.

@jolelievre

jolelievre Nov 28, 2018

Contributor

containS

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 29, 2018

@Quetzacoalt91 @jolelievre Feedbacks implemented in 7525747 😉

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 29, 2018

Travis handle the QA step for this one. Merging.

Thank you @matks

@Quetzacoalt91 Quetzacoalt91 merged commit cc5269a into PrestaShop:develop Nov 29, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.6.0 milestone Nov 29, 2018

@matks matks deleted the matks:new-unit-tests-1 branch Nov 29, 2018

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