Dealing With Testing Fatigue

# .big[Dealing With Testing Fatigue]

##.mtf[Mage Test Fest]

###.big.integergreen[Fabian Schmengler]

---
class: middle, nonumber

.aside.width100[![Hair Schmengler](images/hair-schmengler.jpg)]
.left[
#Fabian Schmengler

Developer and partner at .integergreen[integer_net]

Testing Magento since 2011

]
---
layout: true
.footer[.left[Fabian Schmengler /> **Dealing With Testing Fatigue**] .right[@fschmengler]]
---

# Did you know?

![TDD is dead](images/tdd-is-dead.png)

---

# What I'm hearing a lot:

> I'd really like to write more tests but I don't get any practical value from them. It takes too much time, or it is too hard. Therefore I don't enjoy it or stopped doing it.

(I totally understand that!)
---

# Another popular opinion:

> Integration tests are superior to unit tests. Do not waste your time with unit tests.

(at least they are writing tests!)
---
background-image: url(images/2-unit-tests-0-integration-tests.jpg)

---

<blockquote class="twitter-tweet" data-lang="de">Things are mostly fine <a href="https://t.co/hVzuMRiIfx">pic.twitter.com/hVzuMRiIfx</a>— The Practical Dev (@ThePracticalDev) <a href="https://twitter.com/ThePracticalDev/status/733908215021199360?ref_src=twsrc%5Etfw">21. Mai 2016</a></blockquote>

**Spoiler:** A healthy mix of tests is best
???
for testing the lightbulbs, you don't need a whole ship
---
class: center, middle

#.big[Why all the frustration?]

???
From my observation there are typical problems in tests that could be avoided.

For this presentation I've wanted to write them down and offer solutions.

But of course, everything already has been done before.
---

.aside.width100[![xunit test patterns](images/xunit-test-patterns.gif)]

http://xunitpatterns.com/

#.big[ "Test smells"]

- Project smells
- Behavior smells
- Code smells

???
smells tell you there's something wrong with the test suite

project smells can be observed on project management level

behavior smells can be observed while executing the automated tests

and finally, code smells can be observed in the code.

project and behavior smells are immediately frustrating, and code smells are causing them
---

# Why all the frustration?

#.big[The project smells]

- Buggy tests
  - High test maintenance cost
  - Production bugs
  - Developers not writing tests

???

Developers not writing tests:

- No time?
- No skills and no time to learn?
- Software design not ideal for testing?
- Wrong test automation strategy?

---

# Why all the frustration?

#.big[The behavior smells]

- Fragile Test
- Erratic Test
- Slow Test
- Frequent Debugging
- Manual Intervention

???

CODE smells are less obvious and we'll come to them later
---
class: center
background-image: url(images/what.jpg)

#I've been through it, too...

???
sounds familiar?
---
class: center
background-image: url(images/give-up.jpg)

#**...but did I give up?**

#.mtf.huge.red[NOPE]

#**There's a way out!**

---
class: center
background-image: url(images/stones.jpg)

#.big[**.black[(It's not easy!)]**]

---
class: center, middle

# First and foremost:

#.huge[Learn!]

## Testing is a different skill than coding

???

# How?

- Books
- Conferences
- Katas
- Small projects

Bad news: it takes time and experience

---
class: center, middle

#.big[Test the right things!]

## No, "everything" is not the answer
???

e.g.

- Do not unit test all classes
- Choose sensible boundaries instead. A UNIT IS NOT A CLASS
- Use a *healthy mix* of unit tests and integration tests (other tests help too)
- Always question: are/will these tests provid[e|ing] value? Don't be afraid to delete tests

---
class: center, middle

#.big[Write testable code!]

## If code is hard to test, make it easy to test

???

NOTE: Test-friendly architecture: explained in next slides

---

# Write testable code
#.big[Dependency Injection]

???
we all do this in Magento 2, no need to explain I hope!
---

# Write testable code
#.big[Dependency Inversion]

???
1. High-level modules should not depend on low-level modules. Both should depend on abstractions.
2. Abstractions should not depend on details. Details should depend on abstractions.

if that sounds complicated, remember this *rule of thumb*: depend on interfaces instead of concrete classes
---
class: center, middle

# Write testable code
#.big[Ports & Adapters]
???
I'll throw in "ports and adapters" as keyword, but this could fill a whole different talk, so I'll keep it short
---
class: whitebg
background-image:url(images/ports_and_adapters.png)
???
domain layer provides "ports", application layer is "plugged in" with adapters

the ports are interfaces. *For once* those that are called by the application (e.g. from user interface) *Second*,
    interfaces that must be *implemented* by the application layer to provide the domain indirect access to
resources like database and external web services

module consists of domain layer and thin integration for the adapters
---
class: whitebg
background-image:url(images/ports_and_adapters_unit_tests.png)
???
plug out all external systems, plug in the tests

in unit tests focus on the interface to the outside
---
class: center, middle

#.big[Improve test architecture]

## Know your (anti-)patterns
???

# What's next:

- Typical problems ("testing smells")
- Possible solutions (patterns, ...)

---

#  Test phases

1. **Setup**

set up fixture (instantiate objects)
2. **Exercise**

interact with subject under test (SUT)
   
3. **Verify**
  
    assertions
    
4. **Teardown**

clean up fixture

???
First, let's look at the structure of a test, and some terminology
---

# Fixture setup strategy

- Prebuilt fixture

- Shared fixture

- Fresh fixture

???
to set up the fixture, there are different strategies that you should know to choose the right one for each case
---

# Fixture setup strategy

- **Prebuilt fixture**

e.g. Magento integration test database

- Shared fixture

- Fresh fixture

---

# Fixture setup strategy

- Prebuilt fixture

- **Shared fixture**

- shared by whole test suite

e.g. Magento integration test object manager, Magento unit test code generation
    
    - shared by test case
    
        `setUpBeforeClass()`, or lazy setup

- Fresh fixture

---

# Fixture setup strategy

- Prebuilt fixture

- Shared fixture

- **Fresh fixture**

- Inline in test method
    - Implicitly in `setUp()`

???

...

they all can be useful in some cases. now keep these in mind when it comes to the code smells:
---

# Testing smell

#.big[Copy and paste]

*Duplicated logic in test code*

<!--
$ phpcpd dev/tests/integration/testsuite/Magento
10.30% duplicated lines out of 174717 total lines of code.

$ phpcpd vendor/magento/module-*/Test/Unit
5.15% duplicated lines out of 504438 total lines of code.
-->

```
dev/tests/integration/testsuite/Magento/Customer/_files/`customer.php`
dev/tests/integration/testsuite/Magento/Customer/_files/`two_customers.php`
dev/tests/integration/testsuite/Magento/Customer/_files/`three_customers.php`
dev/tests/integration/testsuite/Magento/Customer/_files/`twenty_one_customers.php`
```
???
First testing smell is an obvious one

...

not only in fixtures. often huge blocks with assertions are copy pasted several times. sometimes with small changes
---

# Problem

- increasing cost of changes

# Possible solutions

**Extract method** refactoring
- In setup:
  - Creation method for fresh fixture
  - Finder method for shared fixture
- In verification:
  - Custom assertion method

---

# Problem

- increasing cost of changes

# Possible solutions

**Extract class** refactoring
- In setup:
  - Test data builder
- In verification:
  - Custom constraint

???
test data builder, a configurable factory
---

# Testing pattern

#.big[Constraint]

```
class IsMeaningOfLife extends \PHPUnit\Framework\Constraint
{
    protected function matches($other)
    {
        return $other === 42;
    }
    protected function failureDescription($other)
    {
        return sprintf(
            "%s is not the meaning of life, the universe and everything", $other
        );
    }
}
```

---

# Testing pattern

#.big[Constraint]

## Usage

```
$this->assertThat(42, new IsMeaningOfLife());
```
---

# Testing pattern

#.big[Test data builder]

Creates fixture, reusable across tests

---

# Test data builder

## Example

```php
CustomerBuilder::aCustomer()
    ->withEmail('test@example.com')
    ->withCustomAttributes(
        [
            'my_custom_attribute' => 42
        ]
    )
    ->build()
```

https://github.com/tddwizard/magento2-fixtures

---

# Testing smell

##.big[Irrelevant information]
???
this one also solves a different problem

when it's not clear which data, in fixtures or in assertions, is actually relevant to the test
---

```
$customer->setWebsiteId(
    1
*)->setId(
*    1
)->setEntityTypeId(
    1
)->setAttributeSetId(
    1
)->setEmail(
    'customer@example.com'
)->setPassword(
    'password'
)->setGroupId(
    1
)->setStoreId(
    1
)->setIsActive(
    1
)->setFirstname(
    'Firstname'
)->setLastname(
    'Lastname'
)->setDefaultBilling(
    1
)->setDefaultShipping(
    1
*)->setRpToken(
*    '8ed8677e6c79e68b94e61658bd756ea5'
*)->setRpTokenCreatedAt(
*    date('Y-m-d H:i:s')
*);
```
???
Magento/Customer/_files/customer_rp_token.php
---

# Problem

- Hard to read test as documentation

# Possible solutions

- Extract creation methods or assertions with **relevant parameters**
 - For fixtures: Test data builder

---

# Testing Smell

##.big[Hard-coded test data]

```
public function testGetTranslationFileTimestamp()
{
    $this->fileManagerMock->expects($this->once())
        ->method('getTranslationFileTimestamp')
        ->willReturn(`1445736974`);
    $this->assertEquals(`1445736974`, $this->model->getTranslationFileTimestamp());
}
```
---

# Problem

- Hard to understand cause and effect

# Possible solutions

- Introduce variables or constants

---

# Testing smell

#.big[Mystery guest]

*Set up or verification relies on information that's not visible in the test*

- Filename
- Database record
- General fixture

???
General fixture: fixture that has been set up not specifically for this test
---

# Mystery Guest

## Filename

```
public function testBundleImport()
{
    // import data from CSV file
    $pathToFile = __DIR__ . '/../../_files/import_bundle.csv';

// ...
}
```
???
Test then references values from the CSV file in assertions
---

# Mystery Guest

## Database record

```
/**
 * @magentoAppArea adminhtml
* * @magentoDataFixture Magento/Reports/_files/viewed_products.php
 */
public function testExecute()
{
    $this->dispatch('backend/admin/dashboard/productsViewed/');

$this->assertEquals(200, $this->getResponse()->getHttpResponseCode());

$actual = $this->getResponse()->getBody();
*   $this->assertContains('Simple Product', $actual);
}
```

???
"Simple Product" in the assertion is a product name from the fixture script
---

# Problem

- Hard to understand cause and effect
- Risk of somebody else editing external source without knowing how it's used

# Possible solutions

- Fresh fixture for each test
- Test data builders instead of fixture scripts
- Create files in test
- Accurately named finder methods

???
risk: several tests use the same external resource, nobody knows for sure which values the other tests rely upon

finder methods instead of properties for general fixtures
---

# General Fixture

## Using attribute
```
$this->assertTrue($this->product->isNew(), 'Product should be new');
```

## Using finder method

```
$this->assertTrue(
    $this->getRecentlyAddedProduct()->isNew(),
    'Product should be new'
);
```

```
private function getRecentlyAddedProduct()
{
    return $this->product;
}
```
???
so *if* you have a general fixture, don't access it directly, but with semantic accessor methods, that refer to the
aspect that is relevant in the current case, like "getRecentlyAddedProduct"
---

# Testing smell

#.big[Eager test]

```
$savedStockItem->setQty(null);
$savedStockItem->save();

$savedStockItem->setQty(2);
$savedStockItem->save();
$this->assertEquals('2.0000', $savedStockItem->load($savedStockItemId)->getQty());

$savedStockItem->setQty(0);
$savedStockItem->save();
$this->assertEquals('0.0000', $savedStockItem->load($savedStockItemId)->getQty());

$savedStockItem->setQty(null);
$savedStockItem->save();

$this->assertEquals(null, $savedStockItem->load($savedStockItemId)->getQty());
```

???
A different smell

Test is verifying too much

Here it looks like we are testing at once

- changing qty from null to int > 0
- changing qty from int > 0 to 0
- changing qty from int to null

---

# Problem

- Hard to tell apart set up, exercise and verification
- Harder to localize defects

# Possible solutions

- Single condition tests
- Create one test case (class) per **case**, not per **subject**

---

# Testing smell

#.big[Excessive Mocking]

$`baseCurrency`->expects()->method('getCode')->willReturn('USD');
    $`baseCurrency`->expects()->method('getCurrencySymbol')->willReturn('$');
    $`baseCurrency`->expects()->method('getRate')->with('AUD')->willReturn('0.80');

$`store`->expects()->method('getBaseCurrency')->willReturn($`baseCurrency`);
    $`store`->expects()->method('getDefaultCurrency')->willReturn($`baseCurrency`);
    $`store`->expects()->method('getAvailableCurrencyCodes')->willReturn(['AUD']);

$this->`storeManager`->expects()->method('getStore')->willReturn($`store`);

???
next smell is unfortunately very common in unit tests

here three mocks with seven mocked methods are created just that the current store has two currencies with conversion
rate
---

# Problem

- Coupling test to implementation
- Test is hard to read

# Possible solutions

- Use real domain objects where possible
    - **Refactor production code**
- Use custom stub or fake objects with simpler setup

???
Avoid coupling of test to implementation!

refactor production code: In Magento most classes have way too many dependencies, simple domain objects almost do not
exist
---

# Testing pattern

#.big[Fake object]

Test specific implementation without external dependencies
---

# Fake object

```php
interface CustomerSession
{
    public function isLoggedIn() : bool;
}
```
```php
class FakeSession implements CustomerSession
{
  private $loggedIn = false;
  public function isLoggedIn() : bool
  {
    return $this->loggedIn;
  }
  `// test methods:`
  public function login()
  {
    $this->loggedIn = true;
  }
  public function logout()
  {
    $this->loggedIn = false;
  }
}
```
---

# Fake object

PHPUnit stub:

```php
$session = $this->createMock(CustomerSession::class);
$session->method('isLoggedIn')->willReturn(true);
```

Custom fake:

```php
$session = new FakeSession();
$session->login();
```

- more succinct
- reusable accross tests

---

# Testing Smell

##.big[Meaningless test names]

```
public function testProcessValidStoreCodeCase1()
{
    // ...
}

public function testProcessValidStoreCodeCase2()
{
    // ...
}

public function testProcessValidStoreCodeCase3()
{
    // ...
}
```

---

# Problem

- Failures in test results are unclear

# Possible solutions

- Write meaningful test names
- Use named keys in data provider arrays
    ```
    return [
        'meaningful description of case 1' => [ ... ],
        'meaningful description of case 2' => [ ... ],
    ]
    ```

---

# Testing Smell

##.big[Meaningless assertion messages]

```
Failed asserting that 0 is 1.
Failed asserting that false is true.
```
---

# Problem

- Failures in test results are unclear

# Possible solutions

- use the `$message` parameter in assert methods to add custom message

???
again, failures are not obvious, especially in CI if the code is not directly available
---

# Testing smell

##.big[Conditional test logic]

```
public function testSend($configValue, $forceSyncMode, $isComment, $sendingResult)
{
    // ...
    if (!$isComment) {
        // ...
    }
    // ...
    if (!$configValue || $forceSyncMode) {
        // ...
        if ($sendingResult) {
            // ...
        } else {
            // ...
        }
    } else {
        // ...
    }
}

```

---

# Problem

- Harder to understand the test
- Harder to write the test correctly

# Possible solutions

- Write separate test cases, extract duplicated logic
- Make sure, tests are deterministic
---

# More smells

- Test logic in production
- General fixture
- Using resources (DB, HTTP) in unit tests
- Indirect testing
- "Inappropiate intimacy"

---

# Conclusion

Maintainability and stability of tests can be greatly improved by taking care of the test architecture.

Watch out for these typical problems. Get rid of them.

.center.huge.mtf[
Make 
Testing 
Fun 
]
???
Jisse: MTF
--
.center.huge.mtf[
Again
]
---
class: center
.huge.mtf[THANK YOU]

--
class: center

##Coming soon:
# Test Driven Magento by Example

---

# Image sources

- Head in sand: [tropical.pete @Flickr](https://www.flickr.com/photos/12023825@N04/) CC-BY-SA 2.0