In #2990511: Add comprehensive test coverage for update status for security releases, we added more robust test coverage for the update module security release tests. In #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already, there is a change to improve those tests to allow it to check for three messages (No update, update available, or security update required) rather than just a boolean for whether or not it says "update available". However, this is out of scope for the bug being fixed.
Add the new test functionality first based on #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already.
+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -445,14 +445,14 @@ public function testHookUpdateStatusAlter() {
* The module version the site is using.
* @param string[] $expected_security_releases
* The security releases, if any, that the status report should recommend.
- * @param bool $update_available
- * Whether an update should be available.
+ * @param string $expected_update_message_type
+ * The type of update message expected.
* @param string $fixture
* The fixture file to use.
*
* @dataProvider securityUpdateAvailabilityProvider
*/
- public function testSecurityUpdateAvailability($module_version, array $expected_security_releases, $update_available, $fixture) {
+ public function testSecurityUpdateAvailability($module_version, array $expected_security_releases, $expected_update_message_type, $fixture) {
$system_info = [
'#all' => [
'version' => '8.0.0',
@@ -465,7 +465,7 @@ public function testSecurityUpdateAvailability($module_version, array $expected_
@@ -465,7 +465,7 @@ public function testSecurityUpdateAvailability($module_version, array $expected_
];
$this->config('update_test.settings')->set('system_info', $system_info)->save();
$this->refreshUpdateStatus(['drupal' => '0.0', 'aaa_update_test' => $fixture]);
- $this->assertSecurityUpdates('aaa_update_test', $expected_security_releases, $update_available, 'table.update:nth-of-type(2)');
+ $this->assertSecurityUpdates('aaa_update_test', $expected_security_releases, $expected_update_message_type, 'table.update:nth-of-type(2)');
}
These changes can be included in the new issue.
+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -505,7 +505,7 @@ public function securityUpdateAvailabilityProvider() {
- 'update_available' => FALSE,
+ 'expected_update_message_type' => static::SECURITY_UPDATE_REQUIRED,
Changes like these should be included in the new issue.
+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -545,9 +545,17 @@ public function securityUpdateAvailabilityProvider() {
+ // On latest security release for module major release 1.
+ // Security release also available for next major.
+ '8.x-1.2, 8.x-1.2 8.x-2.2' => [
+ 'module_patch_version' => '8.x-1.2',
+ 'expected_security_release' => [],
+ 'expected_update_message_type' => static::UPDATE_NONE,
+ 'fixture' => 'sec.8.x-1.2_8.x-2.2',
+ ],
New test cases like this should not be included in the new issue.
+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -165,17 +165,17 @@ public function testMajorUpdateAvailable() {
- * @param bool $update_available
- * Whether an update is available.
+ * @param string $expected_update_message_type
+ * The type of update message expected.
* @param string $fixture
* The test fixture that contains the test XML.
*
* @dataProvider securityUpdateAvailabilityProvider
*/
- public function testSecurityUpdateAvailability($site_patch_version, array $expected_security_releases, $update_available, $fixture) {
+ public function testSecurityUpdateAvailability($site_patch_version, array $expected_security_releases, $expected_update_message_type, $fixture) {
$this->setSystemInfo("8.$site_patch_version");
$this->refreshUpdateStatus(['drupal' => $fixture]);
- $this->assertSecurityUpdates('drupal-8', $expected_security_releases, $update_available, 'table.update');
+ $this->assertSecurityUpdates('drupal-8', $expected_security_releases, $expected_update_message_type, 'table.update');
}
These changes should be included in the new issue.
+++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
@@ -25,6 +25,21 @@
*/
abstract class UpdateTestBase extends BrowserTestBase {
+ /**
+ * Denotes a security update will be required in the test case.
+ */
+ const SECURITY_UPDATE_REQUIRED = 'SECURITY_UPDATE_REQUIRED';
+
+ /**
+ * Denotes an update will be available in the test case.
+ */
+ const UPDATE_AVAILABLE = 'UPDATE_AVAILABLE';
+
+ /**
+ * Denotes no update will be available in the test case.
+ */
+ const UPDATE_NONE = 'UPDATE_NONE';
+
protected function setUp() {
parent::setUp();
@@ -84,14 +99,16 @@ protected function standardTests() {
@@ -84,14 +99,16 @@ protected function standardTests() {
/**
* Asserts the expected security updates are displayed correctly on the page.
*
+ * @param string $project_path_part
+ * The project path part needed for the download and release links.
* @param string[] $expected_security_releases
* The security releases, if any, that the status report should recommend.
- * @param bool $update_available
- * Whether an update should be available.
- * @param $update_element_css_locator
+ * @param string $expected_update_message_type
+ * The type of update message expected.
+ * @param string $update_element_css_locator
* The CSS locator for the page element that contains the security updates.
*/
- protected function assertSecurityUpdates($project, array $expected_security_releases, $update_available, $update_element_css_locator) {
+ protected function assertSecurityUpdates($project_path_part, array $expected_security_releases, $expected_update_message_type, $update_element_css_locator) {
$assert_session = $this->assertSession();
$page = $this->getSession()->getPage();
$this->standardTests();
@@ -105,13 +122,20 @@ protected function assertSecurityUpdates($project, array $expected_security_rele
@@ -105,13 +122,20 @@ protected function assertSecurityUpdates($project, array $expected_security_rele
if ($expected_security_releases) {
$expected_download_urls = [];
$expected_release_urls = [];
- foreach ($expected_security_releases as $expected_security_release) {
- $assert_session->elementTextNotContains('css', $update_element_css_locator, 'Up to date');
+ if ($expected_update_message_type === static::SECURITY_UPDATE_REQUIRED) {
$assert_session->elementTextNotContains('css', $update_element_css_locator, 'Update available');
$assert_session->elementTextContains('css', $update_element_css_locator, 'Security update required!');
+ $assert_session->responseContains('error.svg', 'Error icon was found.');
+ }
+ else {
+ $assert_session->elementTextContains('css', $update_element_css_locator, 'Update available');
+ $assert_session->elementTextNotContains('css', $update_element_css_locator, 'Security update required!');
+ }
+ $assert_session->elementTextNotContains('css', $update_element_css_locator, 'Up to date');
+ foreach ($expected_security_releases as $expected_security_release) {
$expected_url_version = str_replace('.', '-', $expected_security_release);
- $release_url = "http://example.com/$project-$expected_url_version-release";
- $download_url = "http://example.com/$project-$expected_url_version.tar.gz";
+ $release_url = "http://example.com/$project_path_part-$expected_url_version-release";
+ $download_url = "http://example.com/$project_path_part-$expected_url_version.tar.gz";
$expected_release_urls[] = $release_url;
$expected_download_urls[] = $download_url;
// Ensure the expected links are security links.
@@ -119,7 +143,6 @@ protected function assertSecurityUpdates($project, array $expected_security_rele
@@ -119,7 +143,6 @@ protected function assertSecurityUpdates($project, array $expected_security_rele
$this->assertTrue(in_array($download_url, $all_security_download_urls), "Release $download_url is a security download link.");
$assert_session->linkByHrefExists($release_url);
$assert_session->linkByHrefExists($download_url);
- $assert_session->responseContains('error.svg', 'Error icon was found.');
}
// Ensure no other links are shown as security releases.
$this->assertEquals([], array_diff($all_security_release_urls, $expected_release_urls));
@@ -130,14 +153,17 @@ protected function assertSecurityUpdates($project, array $expected_security_rele
@@ -130,14 +153,17 @@ protected function assertSecurityUpdates($project, array $expected_security_rele
$this->assertEquals([], $all_security_release_urls);
$this->assertEquals([], $all_security_download_urls);
$assert_session->pageTextNotContains('Security update required!');
- if ($update_available) {
+ if ($expected_update_message_type === static::UPDATE_AVAILABLE) {
$assert_session->elementTextContains('css', $update_element_css_locator, 'Update available');
$assert_session->elementTextNotContains('css', $update_element_css_locator, 'Up to date');
}
- else {
+ elseif ($expected_update_message_type === static::UPDATE_NONE) {
$assert_session->elementTextNotContains('css', $update_element_css_locator, 'Update available');
$assert_session->elementTextContains('css', $update_element_css_locator, 'Up to date');
}
+ else {
+ $this->fail('Unexpected value for $expected_update_message_type: ' . $expected_update_message_type);
+ }
}
}
All of these changes should be included in the new issue.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | interdiff.2995076.15-23.txt | 837 bytes | mikelutz |
| #23 | 2995076-23.drupal.Let-UpdateTestBase-check-for-multiple-status-messages.patch | 14.31 KB | mikelutz |
| #15 | interdiff.2995076.12-15.txt | 1.07 KB | mikelutz |
| #12 | interdiff.2995076.10-12.txt | 2.11 KB | mikelutz |
| #12 | interdiff.2995076.5-10.txt | 9.59 KB | mikelutz |
Comments
Comment #2
xjmComment #3
benjamindamron commentedStill working on this.
Comment #4
benjamindamron commentedI believe I have everything but the rename of files isn't working:
Will keep looking.
Comment #5
benjamindamron commentedWhat I have so far. The rename doesn't work for me. Can someone try it out?
Comment #6
xjmThanks @benjamindamron!
Comment #7
mradcliffeThe patch applied for me and I saw the new 2.0 files renamed.
I don't have time to review the patch. It is a pretty big patch to review to compare the things that @xjm mentioned in the issue summary to confirm that those and only those are in the patch as compared to #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already. I'll flip this to Needs review to see what the test bot makes of it and for anyone who wants to review the patch.
Comment #8
mradcliffeBut what I can do is run an interdiff between the patch in #90 by @tedmow of #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already and the patch in #5.
Comment #9
xjmThanks, this is really close! There are a few more changes in this patch than we actually need for this issue. The following changes don't need to be included:
(Whole file) This text fixture is for tests for the other issue, so we don't need to add it here.
Oops! I didn't mean for these renames to be included in the scope for this issue. That was a copy/paste mistake on my part in the IS. I will update the summary and let's take these changes out of the patch.
These added hunks are added test coverage that is a part of fixing the other issue #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already and should not be included here. So we can just remove these hunks from the patch.
These docs fixes are either related to the misnamed fixtures or the bugfix for #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already, so these hunks can be removed from the patch as well.
So let's just take out those few parts.
Comment #10
mikelutzI was partially paying attention to the issue at camp. I don't fully understand it, but I can remove hunks pretty easily if this helps.
Comment #11
xjmThanks @mikelutz!
These two bits should be removed as well.
These lines should also remove the rename (of 0.2 to 2.0).
Comment #12
mikelutzComment #13
xjmThanks @mikelutz!
Comment #14
xjmAh just two more bits of the old patch here. Since that's a small fix I think I can make it and still be eligible to be the reviewer. :)
One more bit here. :)
Comment #15
mikelutzI'm just removing what I'm told, I wasn't involved to know exactly what to look for. :-)
Comment #17
mradcliffeThanks, @mikelutz. I looked at the patch in #15, and confirmed that @xjm's comments in #14 were addressed. I did not find any
'fixture' => 'sec.2.0strings in the patch. I also went through #1, #2, #3 #4 and #5 in the issue summary are applied in the patch. Let's see if #15 passes. :-)Comment #20
xjmAh I think I know what needs to fix here. In this one ternary it's the name of the release rather than the fixture. This is why we'll want to rename the fixture eventually, very confusing. :P
Anyway this should pass I think. Thanks @mradcliffe and @mikelutz for finishing this up! Let's just make sure my small change fixes the fail.
Comment #22
xjmAdding credit for Ted as he wrote this test addition originally.
Comment #23
mikelutzI think I change a 2.0 to 0.2 that I shouldn't have. Try this.
Comment #24
mikelutzActually, you beat me to it, lol.
Comment #26
xjmGreat minds think alike!
Comment #27
samuel.mortensonLGTM.
Comment #28
xjmI posted #2995367: Fix update module test fixture names for 8.2.0-rc2 sample data for the fixture name issue.
Comment #29
xjmThis case is interesting because it was a
TRUEbefore instead ofFALSE. That was a bug. The only reason it passed was that the "update available" block was checked in anelseblock for the condition:This makes me wonder if we should remove the outer
else, because otherwise we aren't testing everything we think we are. Thoughts?Comment #30
mikelutzI only have a cursory understanding of this one, but if you are on 0.0 and there is a 0.2 and a 1.2, should it not be update_available => TRUE?
$expected_security_releases is true for this one, so the else block shouldn't come into play, unless I'm reading it wrong. Before, there should have been an update available because the update is on the same minor branch, and now it's specifically a security update on the same minor.
I may be completely off, again, I've only started on this today, and from what I overheard at #MWDS
Comment #31
xjmThere are two different messages that are displayed -- "Security update required!" and "Update available". They should not both appear at the same time as far as I know. So when there's a security update required, there shouldn't be any "update available" message, so the previous test was bugged and not actually testing what it seemed to be.
Comment #32
xjmThis is the diff I'm thinking (plus appropriate reindentation):
That way we run the tests for any expected message regardless of their order in the test, and can't accidentally create a test scenario that doesn't test what it's supposed to. Thoughts?
Comment #33
mikelutzNevermind, I see it. I think that while the logic was iffy before, with this update, it rather fixes itself.
Prior to this, updates_available in the data set was ignored if there were security updates available, but it still checked for the proper showing of 'Security update required' and !'Update Available'. Whether update_available should have been FALSE and not TRUE in the data set could be debatable. There WAS technically an update available, but the page still shouldn't show "Update available" and ultimately the test itself tested for the right text by ignoring that key because the security update trumped it.
It's still testing for the right text now, but the 'expected_update_message_type' is used whether there is a higher security version or not, so I don't see a reason to refactor the else.
The only thing I might say is that we have
If there are security releases and
if there aren't. Since the only two acceptable messages if there are security releases higher than what we are on is UPDATE_AVAILABLE or SECURITY_UPDATE_REQUIRED, we could make the top block look like the bottom and fail if we got anything other than those two in the top block, but that is testing the test, not the code, so I don't know how important it is.
Comment #34
mikelutzI think with that diff, you would hit the fail $this->fail('Unexpected value for $expected_update_message_type: ' . $expected_update_message_type); on $expected_update_message_type == SECURITY_UPDATE_REQUIRED, right?
You already check for the UPDATE_AVAILABLE text in the block above now.
Comment #35
xjmAh, good catch! Thanks @mikelutz for the review.
Setting back to RTBC since I think this sufficiently addresses my questions. I'll commit this shortly.
Comment #39
xjmAlright, committed and pushed to 8.7.x, 8.6.x, and 8.5.x (as testing support for fixes we will also need to backport to 8.5.x for #2909665: [plan] Extend security support to cover the previous minor version of Drupal).
Thanks everyone for the quick turnaround on this issue!