Add integration tests for DataExtensions applied #41
Conversation
.travis.yml
Outdated
| - php: 7.0 | ||
| env: DB=MYSQL CORE_RELEASE=3.6 | ||
| env: DB=MYSQL CORE_RELEASE=3.6 PHPUNIT_COVERAGE_TEST=1 |
There was a problem hiding this comment.
We aren't running coverage in this branch at the moment, this might be a little misleading
There was a problem hiding this comment.
Yeah, it was more a mark in preparation for merging up to master. Can remove if you'd prefer.
| { | ||
| parent::setUp(); | ||
| $manifest = new SS_ConfigManifest(BASE_PATH, true); | ||
| $moduleDoesNotExist = $manifest->moduleExists('bringyourownideas/silverstripe-maintenance'); |
There was a problem hiding this comment.
This seems like excessive boilerplate - the standard is to use class_exists. You will also need to run these checks before parent::setUp() otherwise you'll get exceptions from the fixture factory when trying to load non-existent fixture classes
| { | ||
| $package = $this->objFromFixture('Package', 'otheralerts'); | ||
| $badges = $package->getBadges(); | ||
| $this->assertEquals(1, $badges->count()); |
There was a problem hiding this comment.
$this->assertCount(1, $badges)
|
|
||
| public function testGetBadgesHook() | ||
| { | ||
| $package = $this->objFromFixture('Package', 'otheralerts'); |
There was a problem hiding this comment.
Can you please use Package::class to minimise migration concerns against 2.x-dev?
tests/SecurityAlertExtensionTest.php
Outdated
| $this->markTestSkipped( | ||
| 'Module bringyourownideas/silverstripe-maintenance is required for this test, but is not present.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
See comments in previous test class
tests/SiteSummaryExtensionTest.php
Outdated
| $this->markTestSkipped( | ||
| 'Module bringyourownideas/silverstripe-maintenance is required for this test, but is not present.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
See comments in previous test class. Is three copies of this comment enough of a reason to make an AbstractMaintenanceTest or something? 😆
There was a problem hiding this comment.
I did ponder it - but also thought this very comment (use class_exists instead) might come up, so thought to not spend too much time on it :P
I don't think a simple class_exists call warrants an abstract superclass.
There was a problem hiding this comment.
It's an example of adding maintenance burden, since you need to update the logic in three places before it's even been merged 😄 if you want to leave it there thats fine too
tests/SiteSummaryExtensionTest.php
Outdated
| public function testDisplayColumnsDoesNotIncludePrintingColumns() | ||
| { | ||
| $report = Injector::inst()->create('SiteSummary'); | ||
| $this->assertArrayNotHasKey('listSecurityAlertIdentifiers', $report->columns()); |
There was a problem hiding this comment.
This is a weak test really, can you also check that it is included in one of the other methods, then check that it's not in the printing columns?
tests/SiteSummaryExtensionTest.php
Outdated
| { | ||
| // The themes should not affect the test. | ||
| // Ensure we use the default template supplied with this module. | ||
| Config::inst()->update('SSViewer', 'theme_enabled', false); |
There was a problem hiding this comment.
Probably worth putting this into the setUp method
tests/SiteSummaryExtensionTest.php
Outdated
| $content, | ||
| 'ensure our extension doesn\'t override all others' | ||
| ); | ||
| // Content is translatable, check for the identifying class instead. |
There was a problem hiding this comment.
Yes but we don't normally check for translated text in any unit tests. If you want to you could add _t() calls into the tests (which is done occasionally) otherwise it's not worth mentioning this. Essentially here you're testing that the class is rendered into the template, and that's fine
There was a problem hiding this comment.
So ultimately you're saying I should remove this comment?
NightJar
force-pushed
the
pulls/1.0/mild-integration-tests
branch
4 times, most recently
from
731d3c2 to
91b6d27
Compare
By this module to classes provided by bringyourownideas/silverstripe-mainenance Utilising hook points to alter the report generated by that module, along with adding relationships to link Package info to Security Alerts. These now have some rudimentary tests which at least ensure the extensions are applied automatically iff the requisite module exists.
NightJar
force-pushed
the
pulls/1.0/mild-integration-tests
branch
from
91b6d27 to
a1d6457
Compare
|
I'll merge this up now |
By this module to classes provided by
bringyourownideas/silverstripe-mainenance
Utilising hook points to alter the report generated by that module, along
with adding relationships to link Package info to Security Alerts. These
now have some rudimentary tests which at least ensure the extensions are
applied automatically iff the requisite module exists.
addresses #35 - but does not close it until this is merged, and then merged up into the
masterbranch (is upgraded to SilverStripe 4 support).