Problem/Motivation
In order to convert more tests from WTB to BTB in one big bang conversion we want to get some estimation of what kind of failures might exist.
Proposed resolution
This issue should provide a gigantic patch which does the following and JUST the following:
- Using this tool: https://www.drupal.org/files/issues/browsertest-convert.php_.txt move every WTB class to the right BTB directory
- Change the use statements
- Add traits that the test case needs
- ¡STOP!
Instructions how to work with this patch:
- Apply the patch
- Look at a test failure
- Figure out whether a) There is some missing stuff in BTB, open a follow issue for that. Exclude this particular test for now with a documentation about which issue this is blocked on b) If this test needs manual adaption, try to create issues for them as well
- When you changed anything at the script run the following:
rm -Rf core/modules && git checkout 8.3.x core/modules && php browsertest-convert.php && git add core/modules
Remaining tasks
- Announce this change on https://groups.drupal.org/core after 8.3.0-alpha1 is released.
- Commit this patch on Feb 21st 2017
Announcement text proposal
Title A big chunk of old tests using WebTestBase wil be converted to PHPUnit BrowserTestBase on Feb 21st
As part of the PHPUnit initiative a considerable part of Simpletests will be converted to PHPUnit based browser tests on February 21st 2017. A backwards compatibility layer has been implemented so that many Simpletests can be converted by just using the new BrowserTestBase base class and moving the test file. There is also a script to automatically convert test files in the conversion issue.
Developers are encouraged to use BrowserTestBase instead of Simpletest as of Drupal 8.3.0, but both test systems are fully supported during the Drupal 8 release cycle. The timeline for the deprecation of Simpletest's WebTestBase is under discussion.
Comments
Comment #2
Mile23If you say this:
You end up with this patch.
Which will fail.
Apocalyptically. :-)
Comment #3
dawehnerTry to use https://www.drupal.org/files/issues/browsertest-convert.php_.txt maybe :)
Comment #5
Mile23This time with https://www.drupal.org/files/issues/browsertest-convert.php_.txt
Already I noticed that there are some tests which use traits in the same namespace, which are missing after the move.
Comment #6
Mile23And the patch. :-)
Comment #8
xjmThis is awesome!
Comment #9
xjmSome things I noticed looking at #2 (which is actually a pretty clever way to start out here):
43 like that (I think) already in the tests that managed to report results; I wonder if a temporary BC shiv with ArrayAccess would be useful? No idea whether the object structure is remotely similar.
WTB::drupalGetTestFiles()
has 45 usages. I bet that could live in a trait.That's interesting.
Also noticed a couple things to discuss on #2770549: [plan] Discuss desired API and test suite following deprecation of SimpleTest.
Comment #10
dawehnerThis patch had a bit pointing to
.DS_STORE
@mile23
There is an example.gitignore you might consider using :)
Comment #11
xjmdrupalGetHeader()
.drupalPost()
/curlExec()
/curlInitialize()
look like an interesting chunk of things. (AlsodrupalPost()
is a too-confusing name withdrupalPostForm()
.)Similar to #9.1.
Comment #13
xjmComment #14
Mile23Only confusing if you're used to D7. ;-) https://www.drupal.org/node/2087433
Comment #15
dawehnerWhere was this decision made? While I agree having this particular research issue can be helpful, I am not convinced that going through tests individually (on a per module and for the easy modules) is a bad idea.
Comment #16
xjmEdit: moving my comments to the other issue.
Comment #17
xjm@dawehner, also, I guess #15 was on the wrong issue so let's discuss on #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).
Comment #18
dawehnerApplied the strategy from #2735005-74: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) using the small script below.
This is not complete yet.
Comment #20
xjm#2782309: Refactor File and Image related image field creation logic into a new trait implements some of my feedback from #9.
Comment #21
klausiWe need to trigger the testbot for @dawehner's patch.
Comment #23
klausi@xjm: opened #2784263: Move WTB::assertCacheContext() to AssertPageCacheContextsAndTagsTrait to consolidate assertCacheContext() with Simpletest.
Comment #24
dawehnerI adapted the script to
This produced the following patch. IMHO we should include the conversion script into the patch, so we can see what people changed in it.
Comment #25
dawehner.
Comment #27
dawehnerForgot to pull recently.
Comment #29
dawehnerComment #30
dawehnerExcluded some individual tests more
Comment #31
dawehnerExcluded some more
Comment #34
dawehnerExcluded some more
Comment #36
dawehnerThis time we also convert tests in folders ... which includes ALL of system.
Comment #37
dawehnerSome more exclusions.
Comment #39
dawehnerSome more.
Comment #43
dawehner* Applied the patch from #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests to fix a good couple of test failures
*Fixed emails in BrowserTestBase
* A couple of more exclusions
Comment #45
dawehner* Applied #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3)
* Excluded a couple of more tests
Comment #46
dawehner.
Comment #48
klausiOh, exciting! Thanks for the painful and hard work dawehner. If we can make this green by just converting everything that passes right now I would RTBC it. Even more super cool would be to get this ready before 8.2 releases :)
Comment #49
dawehnerLet's exclude all the other ones.
Comment #51
dawehnerWell, let's do it like this
Comment #53
klausiHm strange, even BrowserTestBaseTest is failing with this patch. Can you check if it passes for you locally with the 2 new test methods it has?
Comment #54
dawehnerJust tried it, and yeah it passes. I guess its related with the applied patches: #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3) or #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests
Comment #55
dawehnerComment #56
dawehnerLet's see how many are left now.
Comment #58
dawehnerFixed some more failures, for example by including every file from #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3)
Comment #59
dawehnerThis one could be green
Comment #61
dawehnerIt is green!
Comment #63
naveenvalechaOnly a question : Why are we skipping the same test two times ?
Retested for 8.2.x, patch not applied, movig back to N/R
Comment #64
dawehnerThe reason why this happened is because I organized them by issue, which blocks a certain test to be converted. There are tests which have multiple blockers, which is the reason they appear twice in there.
I think we should commit it to 8.3.x and then try to redo the same in 8.2.x
Comment #65
Mile23+1 on limiting to 8.3.x and then moving to 8.2.x if possible. 8.2.x changes would probably be non-disruptive, and would be allowed in beta since this is a testing improvement.
Should we keep this script in the repo? Maybe under core/scripts so contrib can use it? If we're going to do that, it'll need some more polish.
There are a bunch of these base classes in the patch that seem to come from nowhere. I talked with @dawehner in IRC about it and they're being copied for backwards compatibility, which is good.
Comment #66
klausiThose new test bases should be almost similar copies from Simpletest, right? Why does the patch not show them as new copy/move? Can you roll the patch again with git --find-copies or -M25% or something? If the code is the same as in the Simpletest classes can we share it via traits?
browsertest-convert.php should not stay in the patch, but we can keep it until the patch is fully polished.
Comment #67
dawehneryeah they are copies. I'll try it next time I roll the patch. Let's get first the two other blockers in.
If we really think its worth sharing it via traits I would hope we don't want to do it as part of this issue. This cannot be achieved via some script, which is kinda the point of this issue :)
Comment #68
klausiComment #69
klausiAdded some more exceptions to the script:
* do not convert Views tests, Installer tests, Module tests etc.
* do not copy Traits because we just want to reuse them from phpunit (this will likely cause some fails because I forgot use statements)
* mark old test base classes as deprecated. That way Git can pick up the moved file in the diff and we just see the actual changes.
Comment #70
klausiOpened #2798097: Move all kernel tests to their correct phpunit location.
Comment #72
klausiFixed use statement and conversion script.
Comment #74
dawehnerLet's fix stuff ... :)
Comment #76
dawehnerMeh, another failing one.
Comment #78
dawehnerThere we go, I forgot to add #76 as well
Comment #80
dawehnerJust a reroll
Comment #82
klausiCool this is green now.
What do we do with this patch now? Make an announcement that we will commit this on for example Sept 28th to 8.3.x and 8.2.x? Or is it to late for 8.2?
If it is too late for 8.2 how do we proceed? Make up another random date after 8.2 is released? Or do we have to wait for the next code freeze phase in March 2017 for 8.3?
I propose that we do this before 8.2 is released to keep momentum, would be cool to get some core committer feedback.
Comment #83
dawehnerLet' discuss that on the main issue.
Comment #84
xjmSo both https://www.drupal.org/pift-ci-job/463727 and https://www.drupal.org/pift-ci-job/463729 include something like:
Behat\Mink\Exception\ExpectationException: Link with label ||m`>&!s found.
Sounds like a classic random machine name vs. random string bug; is there any chance we're surfacing an issue like that?
Comment #85
claudiu.cristeaRandom test failures from #80 seems to be caused by #2808085: Pipe char in locators break Mink and Symfony element search. Occasionally the random string generator adds the pipe in output, then this text/label is used in Mink locators.
Postponing on #2808085: Pipe char in locators break Mink and Symfony element search.
Comment #86
klausiHere is a patch from the phpunit initiative feature branch at https://github.com/klausi/drupal/tree/8.3.x-phpunit
Swapped out Mink for our patched version (#2808085: Pipe char in locators break Mink and Symfony element search) and applied #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests, which failed the testbot. So this is also likely going to fail.
Comment #88
klausiOpened #2814035: Make $modules property protected on BrowserTestBase and KernelTestBase.
Comment #89
klausiOpened #2814047: BrowserTestBase should implement AssertMailTrait for backwards compatibility.
Committed those 2 issues to the branch.
Comment #91
klausiAha, this is the same random test fail as experienced earlier in #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests. Let's find out there what causes this.
Comment #92
klausiRolled in #2820442: SimpleTestBrowserTest has random fails when BrowserTestBaseTest gets larger and updated patches from other issues.
Comment #95
klausiAh, forgot to revert an earlier patch from #2814035: Make $modules property protected on BrowserTestBase and KernelTestBase, done now.
Comment #96
klausiYay, it's green! Queuing a bunch of test runs to make sure we have indeed eliminated the random test fails. Please also review all the issues I linked further above.
Comment #97
dman CreditAttribution: dman as a volunteer and at Sparks Interactive commentedThat is looking *very* green! Good stuff. Applied well for me, and I'm spot-checking a few of the small number tests I'm actually already familiar with.
What can we do to help next? (barring running hours of re-tests locally). I had a look, but can't see any action/help I can provide on the related issues I could see whilst this mega-change is pending here.
Comment #98
klausiPatch rerolled, not other changes.
Unfortunately we saw 2 random test failures: a PHP segmentation fault on PHP 5.5 and Postgres and an update path test fail on SQLite. I don't understand yet if they relate at all to this conversion or not.
@dman: you can help in #2809161: Convert Javascript/AJAX testing to use JavascriptTestBase and #2809181: Provide forward compatibility layer for BrowserTestBase::xpath for example :)
Comment #99
klausiImplemented the automatic addition of AssertMailTrait when the old Simpletest used drupalGetMails(). That means we don't need #2814047: BrowserTestBase should implement AssertMailTrait for backwards compatibility anymore.
Comment #100
dawehnerNice!!!
Comment #101
klausiRerolled after #2808085: Pipe char in locators break Mink and Symfony element search, no other changes.
Now the only remaining dependency is #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests.
Comment #102
klausiWe fixed a random test fail in #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests, merged that in.
Comment #103
klausiThe direction of this issue has changed slightly, we are now also adding traits to converted test classes because we don't want to make BrowserTestBase larger.
Comment #104
klausiMerged in 8.3.x, added 2 tests to the conversion script that cannot be automatically converted right now.
#2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests is still pending, let's hope it gets committed soon.
Comment #105
klausiAnd the last one got committed. Simple reroll, no other changes.
Once this is green we can remove the conversion script from the patch.
We should also announce a date now when we want to commit this large phpunit conversion chunk.
Comment #106
klausiYay, so here is the patch without the conversion script.
This should now be fully ready - please review!
Comment #108
klausiLooks like a random fail in Drupal\page_cache\Tests\PageCacheTest, queuing all environments again just to make sure.
Comment #109
klausiWe have one Drupal\aggregator\Tests\FeedLanguageTest fail complaining that Drupal is already installed (race condition with parallel tests?) and one Drupal\Tests\ckeditor\FunctionalJavascript\AjaxCssTest fail on PHP 7.1.x-dev which is an experimental environment.
So this should be good to go, can somebody double check and RTBC this?
Comment #110
claudiu.cristea@klausi, let's exclude them from this conversion and open 2 follow-ups just to be sure we're not breaking anything.
Comment #111
claudiu.cristeaHm. FeedLanguageTest was not converted and AjaxCssTest is a JS test. Ignore my comment.
Comment #112
dawehnerEither we have an impressive test coverage in Drupal, or we have converted a hell lot of tests in this patch, or both :)
I really like how it covers tests from a lot of subsystems. This should give us higher confidence.
I went through the changes with phpstorm and couldn't see anything wrong. We really just move files / move base classes around.
Comment #113
klausiThanks!
As mentioned in #2807237: PHPUnit initiative we will also announce a date when we commit this conversion to 8.3.x. I propose February 27th 2017, shorty before 8.3.0-rc1 is released. I will poke the core committers now to get a date fixed and announced.
Comment #114
dawehnerA monday would be a good idea IMHO, as there is a lot of time during the week to fix stuff. To be honest though given that RC is this week people might be more stressed out. Maybe a week earlier, aka. the 20th would be a better time.
Comment #115
klausicatch also said a week before RC would be good, so that would match Feb 20th as dawehner proposed. That also works for me.
Comment #117
xjmThat timeline sounds good to me as well! How exciting.
Assuming we go forward with this plan, we can announce it after we release the alpha.
Comment #118
jibranDo we want to enforce no more WTB in new patch policy after this is fixed?
Comment #119
dawehner@jibran
I think we should discuss in a separate issue.
Comment #120
mpdonadioCreated #2847678: Deprecate WebTestBase and its required classes in favour of BrowserTestBase as a f/up.
Comment #121
klausiOK, so let's aim for Feb 20th. And this will go into the 8.3.x branch.
Drafting an announcement text in the issue summary. What else should we mention in the announcement?
Comment #122
mpdonadioA datetime.module class snuck into a recent patch. This removes it (I just hand edited #106 and removed the hunks); will be part of #2780063: Convert web tests to browser tests for datetime and datetime_range modules.
Comment #123
mpdonadioKeep forgetting to tag issues.
Comment #124
klausiOne minor thing:
this looks like an unrelated change, why is it necessary?
Comment #125
klausiSorry, that change has been introduced before in one of my earlier patches. Reverted.
Comment #126
dawehnerI went through the patch again and couldn't find anything else than pure moving. You have good eyes klausi!
Comment #127
xjmI just learned that Feb. 20 is a US holiday and many businesses will be closed. I wonder if we should do this Feb. 21 instead?
Comment #129
klausiRerolled, interdiff is resolving the merge conflicts and rolling back what git wrongly merged.
Comment #130
klausi@xjm: yes, Feb 21st also works!
Comment #131
klausifixing one more date.
Comment #132
dawehnerIt is green, as expected.
Comment #133
mpdonadioI think that after this lands we need #2851541: [policy, no patch] No new WebTestBase tests in Drupal 8 after mass-conversion.
Comment #134
klausiOnly 8 days to go, will ping the release managers again on IRC to get the announcement out at https://groups.drupal.org/core before it is too late.
Comment #135
klausiOnly 4 days to go, I guess it does not make sense anymore to put out the announcement because nobody could react to it anyway over the weekend?
So I propose that we change the announcement to just inform people after the fact that stuff has been converted. I will change the proposed text after this has been committed on Tuesday :)Comment #136
klausiJust saw a response from xjm on IRC that I have now permission to publish announcements myself on https://groups.drupal.org/core . Published the announcement, better late then never! https://groups.drupal.org/node/516229
Comment #137
xjmThanks @klausi! Very sorry I was not able to follow up sooner; it got lost in the flood of things over the past week.
I am super excited!
Comment #138
xjmBTW keeping this issue RTBC so it gets nightly retesting was a good choice; it helps us make sure that we are no longer introducing any random failures that are not accounted for by known issues. I can confirm everything on the issue so far is also present in HEAD because I get emails every time a core branch fails and I've seen them all before. :P
Comment #139
xjmOr, more formally, #2829040: [meta] Known intermittent, random, and environment-specific test failures lists most of them.
Comment #140
klausiThanks a lot for watching out xjm! I really think we should immediately disable all random test failure prone test cases to prevent you getting the same fail emails every day.
Comment #141
jibranI think it is 21st February in every timezone now. :D *fingers crossed*
Comment #142
xjm@jibran Well, for the Americans, committing patches at 2/4/5a is probably not a great idea. ;)
Keeping an eye out now on the queued tests. FWIW, I also confirmed that the conversion does not conflict with our two RC-sensitive BC issues (or reverts thereof), #2751325: All serialized values are strings, should be integers/booleans when appropriate and #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing, since both use our modern testing APIs already.
Comment #143
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!
Comment #145
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #146
mpdonadioThink there is a weird regression with the conversion, https://www.drupal.org/node/2745123#comment-11949935
Will post followup, but I am not 100% sure what is happening to make a real bug report yet, other than something potentially different in the way test teardown works that make the language manager unhappy.
Comment #147
xjmThis way merits an RN mention.
Comment #148
xjm@mpdonadio, did you end up posting that followup? Sounds potentially important?
Comment #149
mpdonadio#148, no I think @daehner, @klausi, and @alexpott are of the opinion that this isn't really related. It has more of the other issue doing something that it shouldn't be.
Comment #151
Mile23Added CR: https://www.drupal.org/node/2999939
And in case you're wondering:
git show --pretty="" --name-only 4c64f7b840ed4cbd1d075b3f40922576bacd9f69 > list.txt
+ a disposable unit test that does lexical parsing.