Problem/Motivation
in #3085717: [PP-1] Do not rely on version_* tags it was determined that release information version_patch and version_extra are not fully tested.
In that issue we want to remove reliance on this metadata from the update xml and instead get the information directly from version number instead.
We should first make sure we have test coverage for the current functionality. Currently these are taken into account for contrib test and may be under tested for core
Proposed resolution
Determine what test coverage we are missing and create it
Remaining tasks
Duplicate \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable()
which test version_patch
and version_extra
for core in \Drupal\Tests\update\Functional\UpdateContribTest
for contrib
User interface changes
none
API changes
none
Data model changes
none
Comment | File | Size | Author |
---|---|---|---|
#19 | 3094304-19.patch | 59.81 KB | tedbow |
#19 | interdiff-17-19.txt | 3.81 KB | tedbow |
#17 | 3094304-117.patch | 59.83 KB | tedbow |
#17 | interdiff-16-17.txt | 15.52 KB | tedbow |
#16 | 3094304-16.patch | 54.3 KB | tedbow |
Comments
Comment #2
tedbowHere is patch that removes the version_patch and version_extra information for releases.
This patch will cause just 1 test fail in
\Drupal\Tests\update\Functional\UpdateCoreTest
and none in\Drupal\Tests\update\Functional\UpdateContribTest
At the very least we need make sure this patch would cause fails in
UpdateContribTest
We should also look at what other test coverage we are missing for the version_* metadata
Comment #3
tedbowComment #5
dwwI don't really understand why we want to add new test coverage for functionality we're about to remove.
Seems like we shouldn't add new test coverage relying on items from the XML feed that we're about to deprecate and stop using entirely. New tests for about-to-be-deprecated stuff isn't going to help us make the new code work better. All these tests would do is slow us down, since we're going to have to modify them or remove them entirely once all the
<version_X>
tags are gone from the updates.d.o feeds we're consuming.I'd rather see the effort go into adding test coverage around parsing the version string and using it properly.
IMHO, this issue sounds like a waste of time and energy, and will only make more work for us moving forward.
Strong vote for "won't fix"...
Comment #6
tedbow@dww sorry I titled/framed this issue incorrectly.
We don't need to test particularly test coverage for the particularly these elements
version_extra
andversion_patch
.We need test coverage for contrib releases like 8.x-1.0-alpha1 and the differences between having multiple updates with different patch versions like 8.x-1.1 and 8.x-1.2.
Right now we are getting that information from
version_extra
andversion_patch
. In #3085717: [PP-1] Do not rely on version_* tags we get that information from the version string itself, which is good idea.But #3085717 how will we know that we didn't break the current functionality around alpha, beta, rc releases? We should know that we didn't break the current functionality around those type of releases because our test coverage should break if we break current functionality. #3085717 should not change the functionality of which updates are displayed to the user just how the information is obtained from the XML.
I am uploading patch that will remove in occurrence of beta, alpha, rc or dev(only beta is actually used) from XMl files that supply the test releases for the contrib modules tested in
\Drupal\Tests\update\Functional\UpdateContribTest
. This removes beta fromversion_extra
but also from the version strings. If we actually had current test coverage for this type of functionality this should break some test coverage inUpdateContribTest
but my local testing show it will not.So I still think we should be clear about current functionality by the way of comprehension test when we do an issue like #3085717 which in itself should not change end user functionality just how it is achieved. Otherwise there is much more of risk that we will break current functionality.
Comment #7
dww@tedbow re: #6: Okay, thanks. That makes more sense. Carry on. ;)
Cheers,
-Derek
Comment #8
Gábor HojtsyDid not fail unfortunately :/
Comment #9
tedbow@Gábor Hojtsy Yep I expected #6 to pass because we currently don't have test coverage for contrib modules to the extent we do core. For instance we don't have anything like
\Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable()
in\Drupal\Tests\update\Functional\UpdateContribTest()
so we don't have the basics test currently for contrib.Comment #10
tedbowHere a few patches
\Drupal\Tests\update\Functional\UpdateContribTest::testNormalUpdateAvailable()
This test will fail if
version_patch
orversion_extra
is removed from the XML it uses.This test pretty much copies
\Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable()
which is the core version#6 proves that currently you can remove
version_patch
andversion_extra
in the test XML and no current tests fail. so we have no test coverage.version_extra
. The new test method should fail.version_patch
. The new test method should fail.This proves that new test need both of these to pass just like the core version
Comment #13
tedbowThe fails in 11 and 12 were expected
Updating the summary
Comment #14
bnjmnmTwo small things
assertResponse()
does not accept a second argument, and is deprecated in Drupal 10 in favor of$this->assertSession()->statusCodeEquals()
(which also does not accept a second argument).version_extra
?Comment #15
tedbow@bnjmnm thanks for the review!
Not if I should add that test coverage to this issue. Maybe because lines near this will be affected in #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates.
but lets see if this patch passes or if it is tested elsewhere
Comment #16
tedbowso the remove_also_releases.patch passed which "Also available" is not test at all for contrib or core ☹️
"Also available" doesn't actually deal with
version_extra
it shows the latest update on the next major version\Drupal\Tests\update\Functional\UpdateCoreTest::testMajorUpdateAvailable()
tests for the next major update for core but in test XML only Drupal 9 is supported so it is not under "also available"So the current test, and
\Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable()
which this was copied from has this problem where it tests for the link to a version and then checks there are actually to versions with update links.One is "Recommended" and one is "Latest" but it doesn't actually test that link is the right one and does test the other at all.
Fixing that. Which also means for the new 8.x-2.0 test case mentioned above this tests for both the "Recommended" and "Also available" links.
\Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable()
needs to a new test for a new major being listed in "Also available" it needs to check the links it is currently testing for actually are under the correct heading, Recommend and LatestComment #17
tedbowI am fixing the core version of
testNormalUpdateAvailable()
so I am moving this method to\Drupal\Tests\update\Functional\UpdateTestBase
It need a couple other changes so that class can provide the update table CSS selector and project name. But also for some reason the
drupal.*.xml
fixtures don't use the version number directly in the download links link the contrib test fixtures do. 🤷♂️So this interdiff just uses new assert on the core test method which adds the extra check that not only the label such as "Recommended" is in the text but the expected download is there.
So was testing that "Recommended" and "Latest" update label where there but only actually check for 1 link and not which label that 1 link was under.
fixed that.
I think this could be done in a follow because it will make the patch much bigger and shouldn't block #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates
Comment #18
bnjmnmI agree that addressing #17.3 in a followup is fine. It should get addressed but does not have to happen here.
All I could find were pretty minor things. I'm going to dig around with Xdebug a little more to be sure of it, but addressing the items below + creating the followup may be all that's left to do.
Would be good to add a comment explaining why the str_replace() is needed.
The same assertion is repeated at the beginning and end of the snippet here.
An 'and' and a few commas will make this a little easier to read
Comment #19
tedbowre #17.3 here is the follow-up #3099825: Test available updates when the current major and next major of core are supported
re #18
Comment #20
bnjmnmI only noticed this because the default value of $download_version was changed -- but realized it would be a problem regardless of the default value.
str_replace will return a string even if $subject is NULL, which means the null coalesce in the parent function will never find a null
. (obviously not a problem for the current tests, but could be for future ones)
Comment #21
bnjmnm#20 was a misdiagnosis on my part, there's nothing wrong there at all. This is RTBC.
Comment #24
Gábor HojtsyIt is lovely to see more test coverage for what we already do. Seeing this being a UI test was a bit surprising but since we want to ensure we provide the same feedback to the user, I am fine with that. Committed! Thanks all!
Comment #25
Gábor HojtsyAdjusting credits (again).
Comment #26
tedbow@Gábor Hojtsy thanks for committing!
Comment #28
xjmBackported to 8.8.x also.