Contributions and (unit/behat) tests

[Martimiz]

Hi all,

At what point should a community contribution to core - or other modules - be accompanied by a test? And who will then test the tests?

What I see now is that sometimes useful fixes to modules seem to get stuck on the contributor not being familiar with the testing platform (a lot aren't). So it's up to the owner or some passer by to provide the test. This can grow to be somewhat of an obstacle.

People will google a problem, find the possible fix, try it out and if it's more then a couple of weeks old, they'll probably just add it to core. I'm often tempted that way - I sort of mastered Unit testing, but now it's behat, which I haven't found the time for yet.

Also I'm not sure at what point a fix really needs testing? Of course not when it's a simple typo, but are there any standards?

Martine

[Daniel Hensby]

IMO, if there's ever a piece of functionality that isn't working as expected and you want to patch it, then you should be providing a test that shows it failing and working after your fix.

If something isn't working as expected and there's no failing test then it means there's no coverage of that scenario and it should be added.

If there's a contributor that's added a patch but can't/won't/doesn't add a test, it's my opinion that a core team member (or another community member) with knowledge of the area of code in which there's a problem should write it for them.

It becomes tricky when there are parts of the system with out a single test (I found this was the case for RequiredFields) and that means writing an entire set of tests and defining your own rules on how you think it should work. This can be a lot of work to cover a rather trivial bug, but is for the benefit of the framework. In this scenario, if the community member isn't willing to write the whole set of tests, then a core dev should be taking on that responsibility.

If there's a bug, there should be a test to ensure it never can regress after your fix.

Dan

[Ingo Schommer]

Sorry, a bit late to the party on this thread. I largely agree with Daniel, unit tests should be standard for contributions. We've documented the PHPUnit setup in SilverStripe, and have created a testing harness which should help to speed up writing them (e.g. the YAML fixtures). There might be an opportunity for devs more familiar with testing to give newer devs a hand on pull request, but the expectation can't be that the core team is somehow obliged to write tests for every contribution. Basically, we all want a stable code base, without one bugfix creating two new bugs, and the way to ensure this is tests.

On Behat, the core team will show more leniency given the effort involved - particularly when there's no existing Behat test coverage for a feature. But for new features with significant UI interactions, Behat tests will be mandatory. It's the only reliable way to ensure they continue working across releases. Unless somebody in the community puts their hand up for manual regression testing on every minor release, that is ;)

Ingo