Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#64 | 2767275-tour-webtests-64.patch | 7.92 KB | andypost |
#64 | interdiff.txt | 1.9 KB | andypost |
#63 | 2767275-tour-webtests-63.patch | 6.02 KB | andypost |
#63 | interdiff.txt | 1.69 KB | andypost |
Comments
Comment #2
klausiPatch.
Comment #3
dawehnerI guess there is no question that this function is public?
What about using $this->assertCount directly here?
assertNotEmpty could be used here
This moving seems to belong into another issue, I guess?
Comment #4
dawehnerComment #5
claudiu.cristeaComment #6
claudiu.cristeaComment #8
dawehnerIs there a reason this is a public method? I'm just curious.
Comment #9
claudiu.cristeaI guess there's no reason for a public method.
Comment #11
claudiu.cristeaTest failures are because, since #5, #2773389: BrowserTestBase::getTextContent() wrongly returns the raw content has been committed.
Comment #12
dawehnerThis issue for example feels like one which should be ideally totally movable.
Comment #14
GoZ CreditAttribution: GoZ at Centarro commentedI make some changes to conversion :
assertText()
orassertLink()
Tour module is not the only one to use PageCacheTagsTestBase, which also is used by EntityCacheTagsTestBase and MenuCacheTagsTest. Both are not converted yet. Converting PageCacheTagsTestBase in this patch is required to make tests success, but some duplicate could be found porting the 2 others tests.
Comment #16
GoZ CreditAttribution: GoZ at Centarro commentedI don't understand why named_exact is used instead of named. This change in WebAssert make other tests fail.
Comment #18
GoZ CreditAttribution: GoZ at Centarro commentedI made mistake on #14 and #16 between linkNotExists() and linkExists() for 'named_exact' , but i don't think it's a good thing to change this assert forcing exact name check.
In this patch, i prefer to remove the link test on 'Translation' label which return a false negative founding 'Interface Translation' term. This was previously fix in #11 asserting link with 'named_exact'. Another way to improve linkNotExists should be to add an '$is_exact' boolean param, but this should be done in specific issue.
In patch, i also fix TourTestBase namespace in following tests since TourTestBase move to
Drupal\Tests\tour\Functional
:Comment #19
klausithis file should be moved to the tests directory and should not be in src/Tests anymore.
Can you make sure that the src/Tests folder is empty when this patch is applied?
Patch does not apply for me because of #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits, can you reroll?
Comment #20
GoZ CreditAttribution: GoZ at Centarro commentedOk
core/modules/language/src/Tests/LanguageTourTest.php
should be moved tosrc/Tests
folder but i think it should be done in another issue.It's strange but LanguageTourTest.php is not concerned by #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits. So no reroll is needed for the moment.
Comment #21
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Tests will be moved in the following patch...
Comment #22
jofitz CreditAttribution: jofitz at ComputerMinds commentedMove tests from core/modules/tour/src/Tests to core/modules/tour/tests/src/Functional.
Comment #24
klausilooks like you forgot to move this file? EDIT: ah, that is the same file again that has already been moved, right?
unrelated change that has nothing to do with this issue. Please only change stuff that is in scope for this issue.
assertEquals() exists on BrowserTestBase now, so this change should not be needed.
Comment #25
jofitz CreditAttribution: jofitz at ComputerMinds commentedcore/modules/language/src/Tests/LanguageTourTest.php
because it is in the language module and this ticket is only about the tour module - is this wrong?assertEqual()
(deprecated) toassertEquals()
so should be needed (although, referring back to 2., it could be argued that this is not within the scope of this issue).Comment #26
jofitz CreditAttribution: jofitz at ComputerMinds commentedReverted the unrelated change.
I haven't moved any of the tests that are outside the tour module (e.g.
core/modules/language/src/Tests/LanguageTourTest.php
) - should I?I have left the
assertEquals()
- should that be reverted toassertEqual()
?Comment #27
klausiWe shouldn't change this line. We have a pass() method on our AssertLegacyTrait with BrowserTestBase now, so the old line should be fine.
We have assertText() now on BrowserTestBase, so this change should not be needed.
same here, we have assertLink() now.
Can you go through all changes in the patch and check if they are really necessary? We got a lot of compatibility improvements in the meantime.
Comment #28
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved all unrelated changes, mainly
[]
back toarray()
and reverting back to deprecated functions.Comment #30
boaloysius CreditAttribution: boaloysius as a volunteer commentedI think in #28 Jo Fitzgerald named patch interdiff and reverse.
Comment #31
boaloysius CreditAttribution: boaloysius as a volunteer commentedComment #33
boaloysius CreditAttribution: boaloysius as a volunteer commentedComment #34
boaloysius CreditAttribution: boaloysius as a volunteer commented[] back to array() and reverting back to deprecated functions.
Comment #35
boaloysius CreditAttribution: boaloysius as a volunteer commentedComment #36
dawehnerThere is still a lot of unrelated changes ...
These are unrelated changes.
Unrelated
Comment #37
jofitz CreditAttribution: jofitz at ComputerMinds commentedI fixed most of these in #28 (hence the massive interdiff), I don't know why @boaloysius ignored that. In his defence, at least his patch didn't fail!
Here's a few more fixed, but I expect there might still be more.
Comment #38
jofitz CreditAttribution: jofitz at ComputerMinds commentedI've found a few more. Sorry for spamming this issue, but I'm finding this is the easiest way for me to do it.
Comment #39
jofitz CreditAttribution: jofitz at ComputerMinds commentedOK, I think this is it now.
Comment #40
dawehnerThe only thing I'm wondering: Could we make another mass moving issue, as some of them seem to be still possible.
I guess this is the most minimal change we can do.
Comment #42
GoZ CreditAttribution: GoZ at Centarro commentedRe-roll patch
Comment #43
klausiLooks like you forgot to move the file. Make sure that you check if all modified test files are at their correct location before you submit the next patch. Thanks!
Comment #44
jofitz CreditAttribution: jofitz at ComputerMinds commented@klausi As I asked back in #26 - as part of this ticket, should we be moving files (e.g.
core/modules/language/src/Tests/LanguageTourTest.php
,core/modules/views_ui/src/Tests/ViewsUITourTest.php
&core/modules/locale/src/Tests/LocaleTranslateStringTourTest.php
) that are not in the tour module? Won't that be done as part of #2756059: Convert web tests to browser tests for language module & #2747167: Convert first part of web tests of views_ui (is there a ticket for moving the tests in the locale module?)?Comment #45
klausi@Jo Fitzgerald: right, let's not touch test files of other modules in this issue. Please remove those changes from the patch.
Comment #46
GoZ CreditAttribution: GoZ at Centarro commented@klausi, just to figure out the workflow for next issues:
In this case, do we have to generate 2 patchs ? One without other modules fixes, which will make tests fail, but the one we will commit at the end of this issue. The other one with other modules fixes so we can pass tests.
Comment #47
GoZ CreditAttribution: GoZ at Centarro commentedOr may be i misunderstand the comment. Here are two cases, please give me the right one, or another if i'm wrong in both.
1/ We should not make any changes in other modules files.
2/ We should only fix dependencies from current issue fix on other modules files. Like i do in #42 patch, using the new fixed namespace in views_ui and language (and why i don't move them)
Comment #48
klausi1) is the path forward. We don't modify tests of other modules in this issue.
Note that the old TourTestBase class must NOT be removed. It is a testing base class which might be used by other contrib modules, so we need to make a copy of it for browser tests and just deprecate it. Sorry, forgot to point that out in my previous review.
Comment #49
GoZ CreditAttribution: GoZ at Centarro commentedNot sure it's exactly what you expected, but i prefer to extend the old TestBase test with the new Browser test class instead of copy code.
Comment #50
klausiThanks, but we cannot change the old test base class, it must remain unmodified. It must stay a Simpletest base class, cannot inherit from BrowserTestBase. We can only deprecate it.
Comment #51
GoZ CreditAttribution: GoZ at Centarro commentedComment #52
klausiBefore the @deprecated tag should be an empty line. See AggregatorTestBase for an example. Let's use the exact same wording as there: "@deprecated Scheduled for removal in Drupal 9.0.0.
* Use ... instead."
And I don't think we need the @see reference to this issue.
Nice find! This class was moved but we forgot to change the base class, so this change is correct.
why do we change this line?
This should be detected as copy in git. Can you roll the patch again with --find-copies-harder on git diff?
Comment #53
GoZ CreditAttribution: GoZ at Centarro commented@klausi in #52 3.
I explain this in #18
Comment #54
klausiBut that means we are losing test coverage if we just remove this check.
Instead, we have 3 options:
1) change the assertNoLink() calls in this specific test.
2) change the assertNoLink() implementation on AssertLegacyTrait so that it matches the behavior of Simpletest as good as possible.
3) change WebAssert::linkNotExists() to the named_exact solution from #11.
I think option 1 is a no-go because future Simpletest conversion issues should not run into this exact same problem again. So we should open a new issue to either fix assertNoLink() or WebAssert::linkNotExists().
Comment #55
GoZ CreditAttribution: GoZ at Centarro commentedI open an issue for the assert link part #2860175: Add exact option to AssertLegacyTrait::assertLink(), WebAssert::linkExists() and WebAssert::linkNotExists()
Comment #56
klausiThanks, postponing on that one.
Comment #57
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedAs discussed offline with @klausi the ViewsUITourTest.php (core/modules/views_ui/src/Tests/ViewsUITourTest.php) is moved into scope for this issue.
Comment #58
boaloysius CreditAttribution: boaloysius as a volunteer and at Google Summer of Code commentedComment #59
jofitz CreditAttribution: jofitz at ComputerMinds commented@boaloysius why are you leaving empty comments on lots of tickets? My inbox is getting spammed!
Comment #60
boaloysius CreditAttribution: boaloysius as a volunteer and at Google Summer of Code commented@Jo Fitzgerald last night I was updating the organization I am working for. Sorry.
Comment #61
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #62
larowlan#2860175: Add exact option to AssertLegacyTrait::assertLink(), WebAssert::linkExists() and WebAssert::linkNotExists() is in
Comment #63
andypostUpdated patch
- changed link test to exact
- added deprecation trigger as other deprecated tests do
Comment #64
andypostConvert remaining tests that using deprecated
\Drupal\tour\Tests\TourTestBase
Comment #65
dawehnerThis looks clean enough :)
Comment #68
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!