Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Steps to reproduce:
- Enable testing module (simpletest)
- Go to the UI and select 6 or 7 unit tests (regular functional tests are OK)
- Press "Run tests"
Expected output
The tests will be executed and I see the results
Actual output
Uncaught PHP exception:
Uncaught PHP Exception Drupal\Core\Database\DatabaseExceptionWrapper:
"SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'test_class' at row 1: INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, file) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7); Array
(
[:db_insert_placeholder_0] => 1
[:db_insert_placeholder_1] => Drupal\Tests\commerce\Unit\Resolver\ChainCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\ChainLocaleResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultLocaleResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\ChainStoreResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\DefaultStoreResolverTest
[:db_insert_placeholder_2] => fail
[:db_insert_placeholder_3] => PHPunit Test failed to complete
[:db_insert_placeholder_4] => Other
[:db_insert_placeholder_5] => Drupal\Tests\commerce\Unit\Resolver\ChainCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\ChainLocaleResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultLocaleResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\ChainStoreResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\DefaultStoreResolverTest
[:db_insert_placeholder_6] => 0
[:db_insert_placeholder_7] => .../sites/default/files/simpletest/phpunit-3.xml
)
" at .../core/lib/Drupal/Core/Database/Connection.php line 676, referer: http://localhost/admin/config/development/testing
Proposed resolution
Remove optimisation of trying to run PHPUnit tests before starting the batch. Now we have so many PHPUnit test types this results in timeouts causing the above error.
Remove is_subclass_of()
from SimpletestTestForm::submitForm() - this could potentially autoload all tests in the same request which just does not work.
Remaining tasks
User interface changes
None
API changes
None - a BC layer is added to simpletest_run_tests()
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#73 | 2575725-73.patch | 7.99 KB | alexpott |
#73 | 63-73-interdiff.txt | 1.24 KB | alexpott |
#69 | 2575725-69.patch | 13.36 KB | alexpott |
#69 | 63-69-interdiff.txt | 1.24 KB | alexpott |
#65 | uncaught-php-exception-2575725-65.patch | 7.96 KB | pashupathi nath gajawada |
Comments
Comment #2
alauzon CreditAttribution: alauzon as a volunteer commentedIt also happened to me only when I was running the test with UI in HTTPS mode. In HTTP mode it was not doing it.
Comment #3
dawehnerDoes the same thing happens if you run things via the command line? I'm just curious here.
Comment #4
alauzon CreditAttribution: alauzon as a volunteer commentedIt's worse. On the command-line I've got a way more Exceptions than with the UI. I did not have time to check why it was that way so I went the UI way.
Comment #5
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedThis is because
test_class
isVARCHAR(255)
. With namespaced test classes, that really isn't going to hold many tests! I found it just running the Page Manager/Panels tests (16 tests). The same issue exists withfunction
...Changing them to
TEXT
(which also involved settings a size on thereporter
index's use oftest_class
seems to resolve this for me. I've attached a patch, but I don't know really know what's happening well enough to know if changing toTEXT
(fromvarchar_ascii
which is a special machine name case) or changing the key length is going to have other consequences - so definitely needs a review from someone who does know!Comment #7
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedOops, think that was just using
TEXT
rather thantext
...Comment #9
hedrickbt CreditAttribution: hedrickbt commentedretracting my comment.
Comment #11
Torenware CreditAttribution: Torenware as a volunteer commentedSaw this today with 8.2.x.
Both the test_class and function fields can take a comma delimited list of test_classes , and of function, which will be even longer.
This field really does need to be a TEXT type field; varchar(255) just won't work with namespaced classes.
I got this, BTW, just by doing the following:
Comment #12
Torenware CreditAttribution: Torenware as a volunteer commented@dawehner in #3: this is specific to the Testing form page. run-test.sh does NOT have this issue.
Comment #13
lokapujyaLets try using the text field, without changing the index. I don't know why the index needs to change.
I couldn't test this locally, so trying it out here.
Comment #15
lokapujyaAccording to the test errors, text can't have a default value.
Comment #16
lokapujyaA text index Needs to have a prefix length. All tests are passing so I guess that Drupal assigns a default length. I'm going to in-revert the index length, so that we are explicitly setting a length.
Comment #17
albertski CreditAttribution: albertski at Xeno Media, Inc. commentedPatch looks good and it ran perfectly for me.
Comment #18
alexpottThis needs an update path for the schema changes so sites which have simpletest installed are fixed.
Comment #19
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedAdded hook_update.
Please Review.
Thanks!!
Comment #21
albertski CreditAttribution: albertski at Xeno Media, Inc. commentedCan you update the function description to follow Drupal Coding standards:
Can you also double check that it should 8102. It may be correct I'm just not sure if it should just be 8100.
Comment #22
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commented@albertski ,
changed the function description as per coding standard.
for 8100 and 8101 it was not showing any updates.
So, I used 8102 for which it was working.
Thanks!!
Comment #24
Torenware CreditAttribution: Torenware as a volunteer commentedI think we're doing the wrong thing here. simpletest.install declares hook_schema(), so we need to do something more like https://www.drupal.org/node/2535384. Note db_change_field() is now deprecated.
Comment #25
dawehnerWell, we can indeed replace
db_change_field()
byDatabase::getConnection()->schema()->changeField()
Comment #26
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedI removed db_change_field. and followed the steps given in the link in #24.
Thanks!!
Comment #28
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedI am not getting why the tests are failing.
Please can anyone guide.
thanks!!
Comment #29
Torenware CreditAttribution: Torenware as a volunteer commentedLet me see what we can see here. I did the simplest possible test here: I just applied your patch and ran "drush updb". Here's what I get on a fresh install with simpletest enabled:
This looks to be something @lokapujya found in #16. I'll see if this is in fact the case.
Comment #30
Torenware CreditAttribution: Torenware as a volunteer commentedLooks like our problem relates to the index on ('test_class', 'message_id'), which needs to be dropped before we can modify the field. The update executes w/o error. I'm going to see if this makes the rest of the errors go away as well.
We still need a test of the update, once this works. We may also need to do a little tweaking for Postqres compatibility IIRC.
Comment #31
Torenware CreditAttribution: Torenware as a volunteer commentedPatch implementing the index drop and re-add. Again, not complete w/o test, but this does run locally both from an install and from an update. Letting the testbot get a taste.
Comment #32
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI had this problem when running several tests from the Rules module, but with Devel, and also on Scheduler (which I maintain), I can run many more tests without getting the problem, and without any patch.
In the failed sql insert, all the classes were comma-concatenated into one long string which is inserted into the simpletest table at the start of the test run. With Rules, selecting the first five tests in one run is ok, as the total length is 243 (although it seems to fail with "no results to show"), but picking the first six causes the insert error as this makes 288 chars. But with Devel and Scheduler, only one test class is inserted into the table at a time, when each class starts a row gets added (which is subsequently deleted when that test completes) then the next test class is added. Thus we get no failures and I can run all 16 test classes in one run, each with an average length of 50 chars making 800 chars in total.
So why do we get the differing behaviour? I expect there is a simple solution - be we need to understand it, because just increasing the test_class field length may not be the proper solution if it is meant to only hold one class at a time, as 255 would be enough for that.
Jonathan
Comment #33
Torenware CreditAttribution: Torenware as a volunteer commentedCorrect. This SQL gets generated when using the admin/config/testing page.
Not sure why. I'm assuming that you're using admin/config/testing in all these cases. I'd guess that you're using the interface slightly differently, and therefore getting a different code path in the submitForm() handler in its form class. But if you read the code, I don't see why changing the module is relevant in and of itself; the processing code does not even really "know" what module is posting the tests to the DB.
Comment #34
Torenware CreditAttribution: Torenware as a volunteer commentedA question about creating the appropriate test here.
The relevant manual page seems to be https://www.drupal.org/node/2536494. Part of the process is to create a DB-neutral dump file, and run the installer against it:
Looking in Core, I see that there are a variety of "DBND"s in core/modules/system/test/fixtures, but no where else. I assume that system/tests/fixtures/update/drupal-8.bare.standard.php.gz is a "moving target"; its content should change over the dev cycle. For the test to work, I need to use a dump that reflects the state of things before my schema change.
So, I'd guess I should use the standard database neutral dump. But at my patch level, if I enable the simpletest module, note that simpletest_schema() will have run according to my new state and will reflect the update level subsequent to the update level I need to test. So I need a state of the schema that's not defined in either the base file (because simpletest is not in the standard profile) and not in the patch's simpletest.install level.
The manual page does not cover this case. I could, of course, create my own neutral dump that reflects the old simpletest.install, and simpletest being pre-enabled. But this is about 135K worth of fixture to check in.
What's the recommended thing to do here?
Comment #35
Torenware CreditAttribution: Torenware as a volunteer commentedRE #34. The manual does not make this clear, but it sounds like the test needs to:
This is bit strange, but am I getting this right here? This is an unusually "meta" kind of test.
Comment #36
Torenware CreditAttribution: Torenware as a volunteer commentedTest has been added. The test works, in that it verifies that the update actually ran, and that the module version actually increased. I'm not sure if pulling the schema and examining it actually verifies that we made the changes we thought we made (i.e., the fields are now text rather than varchar_ascii), since if I could do a PDO-equivalent of MySQL's "desc", that strikes me as more probative. Any suggestions on that score?
Comment #37
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedIf there isn't an abstraction level means of checking the changes have taken effect, we could always try inserting something with length > 255 which would throw an exception if it were too long (ie hadn't changed).
Comment #38
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedJust done some more research into my questioning in #33
@Torenware - I am using admin/config/testing UI in exactly the same way for both cases. I have discovered what make the difference. Normal indivudual simpletests run one at a time and you can execute as many as you want with no limit. It is only when the tests are PHPunit tests when we get the problem, as this is the scenario where the individual test classes are comma-concatenated. I am still unsure if this is exactly what is intended to be done with this table. If you select a small number of unit tests (for example in core Views_UI pick the first three test, which are unit tests)
then the SQL exception is not thrown because we have
and the combined length is 148. However, a new exception is thrown by the line in
and we get
This does not look right, to try to use a concatenated list of classes in the
new \ReflectionClass
statement.If you comment out this line, and fake the next using
$doc_comment = '* @group xyz */';
so that the unit tests actual get past this point, you end up with no results, and an output:So we need to know exactly what the purpose of the concatenated class list is for, when inserted into the {simpletest} table, before we have confidence that just increasing the character length is the right tihing to do.
Comment #39
Torenware CreditAttribution: Torenware as a volunteer commented@jonathan1055 -- it sounds like you have an understanding of how the test list gets generated, although I'm not sure if it's in scope for this particular issue. Certainly, it sounds like changing the the field definitions from varchar_ascii to text prevents a crash whichever code path you go over here.
I don't see any of the possible paths you describe as "wrong", as long as tests you intend to run get run, and as long as we don't get the spurious exception described in this issue. So do we really need to modify that code? It certainly works. And if it isn't broken, well, you know how the old expression goes.
Comment #40
Torenware CreditAttribution: Torenware as a volunteer commentedAlso, I'm not sure from your description
as to how to reproduce this issue. It's not the code path we hit earlier in this issue, since we died in the database code.
Certainly, calling
new \ReflectionClass()
on a list of classes is wrong. But it's not the error originally reported in the issue; moreover, that code was wrong, as you do point out, even if the length of the $classname is under 255 ascii characters.On the fairly safe assumption you can describe how to reproduce this problem, might this be better reported as either a new issue, or as a follow-up to this one?
Comment #41
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI was not suggesting to change any code. I was showing that PHPunit tests currently do not run and they produce more exceptions. The fake $doc_comment I had to introduce was to get around another exception where
$annotations['group']
is empty. This was all to show that even when you avoid all exceptions you still get no results.As I showed in the first screen-shot, just select the first three tests in the views_ui group. This avoids the sql 255 limit exception but shows that running any PHPunit tests via the admin UI is buggy and fairly broken anyway.
The purpose of going to this detail was to show that even with your change to increase the table field length, you will immediately hit this next problem.
The whole point I am making is that the original exception reported on this issue is only caused when running PHPunit tests, not for normal functional tests, and I wanted to show that PHPunit tests still fail badly anyway, when you run a small number which do not cause the original exception. Thus we might be addressing the wrong problem by solely focussing on the table field length. I would not like to see you wasting your effort in increasing the length and writing the associated update function, and a test for that function, if the whole PHPunit functionality needs to be altered. It might be that the proper fix for PHPunit tests removes the comma-concatenation and the length increase was entirely unnecessary. Then we will have added extra code and complications for no actual benefit.
Comment #42
alexpottIs simpletest enabled in the filled standard install? If it is - then that'd be preferred to doing the emulation.
This will break the moment we add another hook update N - I think the best way to test this is too run the same query before and after - before should fail and after should pass.
I think this index length might need adjusting for 4byte UTF-8. I think it might have to be 191. But also how is this index actually used? It seems weird to be dumping a really long aggregated string with all the classes and then be using an index...
Comment #43
alexpottThis query in SimpletestResultsForm would benefit from an index on test_id... this index does not help...
The query in simpletest_last_test_get() does not use the index either... again a test_id index would help...
The query in simpletest_script_load_messages_by_test_id() also does not use it ... and yet again a test_id index would help.
The query in simpletest_script_open_browser() also does not use it... and yet again a test_id index would help.
The query in simpletest_clean_results_table() also does not use it... and yet again a test_id index would help.
The query in TestBase::deleteAssert() does not use it.
And that is all I can find. Afaics we should drop the index - it is useless in core. And we should file a follow up to add an index on the test_id column.
Comment #44
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedExcellent review and investigation @alexpott, thank you. This all lends weight to my argument in #38 and #41 that we should not be doing anything to fix/increase the 255 char limit until we know why the test classes are comma-concatenated, what purpose it serves, and whether this code will stay, as there are many other problems with running PHPunit tests via the simpletest UI.
I have found the following - and any of these might render the increase length obsolete:
#2566767: Don't allow running phpunit-based tests via the UI
#2608532: Simpletest UI munges PHPUnit-based test output
#2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate
#2643624: Fix SimpletestPhpunitRunCommandTest
Comment #45
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedComment #46
Torenware CreditAttribution: Torenware as a volunteer commentedI'll decompress it and see. But if yeah, can do.
How best can I test if the field has in fact updated? The test of update level was in any case a proxy for what I really want to do: check that the table schema changed.
The only thing I can think of is to put in string longer than 255 bytes and see if it throws. Is this the right kind of approach?
It seems likely per your next comment that this index is not doing anything useful we know about, so the approach is to simply remove it and not create it (and add one to test_id, which may well be helpful). If we're wrong, and the index is needed for some reason, hopefully the full test suite will smoke this out.
I can't see why anything in contrib would care about this index either, so we're likely safe in doing this.
Comment #47
Torenware CreditAttribution: Torenware as a volunteer commented@alexpott: I'm running into an odd problem using drupal-8.filled.standard.php.gz. Even if the only update in simpletest.install, run-tests.sh fails:
The failure output looks like this:
If I change the
setDatabaseDumpFiles()
to use the bare dump, it works:with output
There's only one test I can find in Core that uses drupal-8.filled.standard.php.gz, which is
Drupal\system\Tests\Update\UpdatePathTestBaseFilledTest
. I'm not sure why it works where the trivial case won't work from me here; I tried inheriting from the same class it does, and I still get the same error.I'm going to go back to using the bare version for now. I'm not sure if I'm doing something subtly (or unsubtly) wrong here, but I think I may have found an issue with using the filled dump.
Comment #48
Torenware CreditAttribution: Torenware as a volunteer commentedI've put in Alex's suggestions, with the exception of the use of the filled dump file. While the filled dump does indeed enable simpletest, I'm still not getting it to work. I've also not added an index for test_id yet. If I do: how best to test that an index has been created in updater?
I can of course add the index on test_id if y'all think this is in scope.
Comment #49
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi Torenware,
Just checking that you have seen my comments in #41 and #44 as you have not responded to them yet. If you want to go ahead anyway, and ignore the likely case that you are wasting a huge amount of your time on this, and adding unnecessary and complex code, that's OK with me, just say that you acknowledge you've seen my comments, and I will stop hassling you about this issue ;-)
One good thing is that I'm learning alot from your coding and commentry, so that's a nice side-effect anyway.
Jonathan
Comment #50
alexpott@jonathan1055 my guess is that the class list is used to determine what tests to run :)
In other news I can not repeat the test fail - I've selected way more than 30 tests with class names that add up to way longer than 255 characters.
Does anyone have a way to reproduce the test fail with only core tests?
Comment #51
alexpottI've done the steps in #11 and nothing blew up. @Torenware are you sure it blew in the same way as reported in this issue? I can't see anywhere in the code it would insert a list of classes like this.
Comment #52
alexpottAh here it is - so it only happens if a PHPUnit test fails. This is pretty bad code since it is not telling you which PHPUnit test failed :(
Comment #53
alexpottI've created #2748315: Fix indexes on simpletest table to fix the indexes.
I think we need to consider the special handling of phpunit - whilst it is nice running them all at once - not knowing which one failed sucks. And to have another fail that hides the failing test really really sucks.
There are moves to make the simpletest UI a command line builder rather than an actually runner which would solve this. But that does not help people in the interim. We could kludge and go the way of the current patch - or we could just remove the ability to run all the phpunit tests at once and run them one by one through the batch. Imo this is the way to go because now far longer tests are becoming PHPUnit so people might hit this just through max execution time.
Comment #54
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@alexpott, To answer your quesion in #50, select six unit tests in the 'views' section. Note this is "views" with lower case, not "Views" with upper case. [edited to say 'six tests', as the screenshot only had five ticked]
Yes that is it. I did all this analysis back in #38, and listed those other issues which all highlight all the problems with running PHPunit tests via the simpletest UI. That's what I have been trying to tell you all along. Concatenating the class names is not used anywhere, and also causes other problems, so it's quite likely that this part of the code will change radically or become obsolete. I see no benefit in changing the schema here and writing the update function, before those other major issues are resolved.
If you follow my repro steps in #38 you will find out just how broken the PHPunit test runs are.
This patch won't solve the problem, it will just show up the next exception - as I explained in #38
Comment #55
Torenware CreditAttribution: Torenware as a volunteer commentedI was using 8.2.x when I saw this. Just tried it; still happens. I get the "The website encountered an unexpected error." white screen, with this error in the apache log:
I'm pulling up the Testing page, and checking off the "simpletest" module line, which selects all of the tests for that module. This suffices to trigger the problem. I'm not sure if the behavior is the same in 8.1.x, however.
I'm guessing those lines get invoked if phpunit out and out crashes -- note the fake test_id. The fails do get logged, but I think it's in
simpletest_phpunit_testcase_to_row()
. Which is confusing as all hell, to be sure.So, yeah, you do know which PHPUnit tests failed. But since it's taking a lot of time to figure out how the code is doing that, yeah, there might be some issues with code ;-)
So the behavior is less broken than you thought, but it's still nothing to rejoice in.
Comment #56
Torenware CreditAttribution: Torenware as a volunteer commented@jonathan1055,
I have seen your comments, but there are two significant issues with them:
As we've seen, the code used to dispatch tests and show their results is very knotty, is hard to understand, and likely needs to be factored enough so that it's easy to diagnose and discover problems with it. So it's not surprising that there are multiple bugs that might surface, or that might be hidden by other bugs. But it makes sense to report the issues with good reproducible bug descriptions. You should definitely do that. But perhaps not in this particular issue number.
YMMV, but that's why I haven't referred to this since #39 and #40.
Comment #57
Torenware CreditAttribution: Torenware as a volunteer commented@jonathan1055 -- OK, now I've seen it. But I can only get it to happen on a completely clean install, which is likely why I hadn't seen it up to now. If I recreate my install from scratch, and do the procedure from #11 with the patch from #48 applied, I'll get your crash. But if I rerun the procedure, no crash. Since I'm using drupalvm, rebuilding is easy, at least. But to see your crash, I need a clean database.
Yeah, we need to understand what the rationale is for putting multiple test classes on a single result object. I can certainly split out the test classes in
SimpletestResultsForm::addResultForm()
, which might prevent the crash. But who knows if that actually makes sense.This looks to be an example of something I've discussed with Ryan Aslett aka Mixologic concerning the testbot. If PHPUnit gets hit with certain kinds of errors, the process just dies, and essentially nothing useful comes back to a parent process that has forked a phpunit child process. I think what we're doing here is passing the full set of phpunit tests to a master phpunit process that itself forks the listed tests as children. If one of the children blow up, it takes down the parent with it, w/o passing back any information concerning how it died; the information should be in a JUnit style XML file, which doesn't get built in this case. So our test runner only knows what it passed to the exploded phpunit processes -- which is the list of tests delegated to phpunit.
As alexpott points out, this case ends up dumping these mystery concatenated test_class values. Since you can't tell what died, they share a sort of truncated result object, so if we split out the concatenated classes, you can at least display something. But all of the listed classes will display as errors, which is not very useful.
Probably a good issue for Alex to triage and figure out what's salvageable here. From what Mixologic told me, it sounds like the problem is a known PHPUnit issue which has yet to be fixed upstream. PHPUnit just doesn't fail gracefully right now. Not sure what we can do about this.
Comment #58
alexpottI think the thing to do here is to remove the PHPUnit optimisation and just run them one by one. Therefore we'll never get into concatenating classnames.
Comment #59
alexpottThis removes the false optimisation for PHPUnit - now we have so much more PHPUnit testing the idea that we should run them all before the batch starts feels wrong. This patch just makes them part of the regular simpletest batch run. Yes we can argue that the simpletest UI shouldn't run phpunit tests and we should just provide a CLI command for people to run but atm we have the UI so let's make it work.
@Torenware I don't think there is anything upstream to fix - this issue occurs because of how we've integrated phpunit into the Simpletest UI - building UIs is not really PHPUnit's concern.
There's no interdiff because this is a different approach to solve the problem.
Comment #60
dawehnerI'm a bit sceptical that we need this BC layer, this function is really just called from the submit function, but well, if you think its useful...
+1 for not executing hooks for phpunit tests.
Comment #61
dawehnerWe can remove the BC later, if we really need to.
@alexpott
Seriously, amazing fix!
Comment #63
alexpottWow the form was setting SIMPLETEST_BASE_URL environment variable. That really shouldn't be happening there. I think we should set it to the $base_url global if it is set.
I've tested this with run-tests.sh and it works fine - that doesn't set the global.
Comment #64
dawehnernitpick: tests can browse, instead of can browser
Its confusing that we copy the old base url over, but don't need it for the DB.
Comment #65
pashupathi nath gajawada CreditAttribution: pashupathi nath gajawada as a volunteer and at Melity commentedPlease find the updated the patch as suggested in #64.
Thanks,
Comment #66
dawehner@pashupathi nath gajawada Please provide an interdiff next time. Thank you.
Comment #67
pashupathi nath gajawada CreditAttribution: pashupathi nath gajawada as a volunteer and at Melity commentedSure @Daniel Wehner.
Comment #68
Torenware CreditAttribution: Torenware as a volunteer commentedI may be misquoting/misdescribing what Mixologic told me. This was while I was trying to figure out why my traits as used in phpunit tests were killing the test bot. But my sense is by dispatching phpunit tests directly, as you do in #59, would get around that problem even if I did understand him correctly.
Comment #69
alexpott@dawehner re-resetting the environment variable to the existing value - I think I was being overly defensive here - just worrying about changing a user's currently set value. But reading the docs...
And see the output of:
@pashupathi nath gajawada thanks for trying to fix #64.1, just to note - your patch includes quite a few whitespace errors.
Patch attached is based on #63 and rewrites the comment completely that #64.1 pointed out was wrong.
Comment #71
dawehnerAh, so this doesn't override the value defined in phpunit.xml.dist?
Comment #72
alexpott@dawehner hopefully this code is never called from phpunit ;)
Comment #73
alexpottRebased patch on latest 8.2.x
Comment #74
dawehnerOh yeah that's a good point.
Comment #76
alexpottUnrelated test fail cause - #2749955: Random fails in UpdatePathTestBase tests.
Comment #79
catchNeeds to be summarize. Fixed that on commit though.
Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!
Comment #80
alexpottDamn I'm losing my fight against the zed's :D
Comment #81
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@Torenware - thank you for seeing my point, and for all your input.
@dawehner - great reviewing
and finally ... @alexpott Awsome change! You really sorted out this problem - thank you!
Comment #82
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedChanged the title to reflect the actual change made, as this issue is linked and referenced from several other places.
Comment #83
klausiHm, I don't agree with the title change.
The PHPUnit tests are our regular tests, the Simpletests are the odd ones that will all be converted and removed. See also #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase). Which reminds me to open an issue to completely remove Simpletest UI, because developers run their tests from the command line or IDE. We are not in the business of maintaining a graphical test runner, it is a waste of developer time.
Comment #84
klausiHere we go: #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner
Comment #85
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@klausi OK, fair enough, I used the wrong terminology. But I think the title should reflect what was actually changed, rather than simply state 'uncaught exception', particularly as the final solution was a structural change.
Just to record it explicitly here: When using the UI, the PHPunit tests are now executed one at a time, via the same batch processing and loop that runs the simpletest webtests. Function
simpletest_process_phpunit_tests()
is called separately for each PHPunit test class selected, instead of being called just once with all the test classes. When the tests are executed viarun-tests.sh
then the process is unchanged and all test classes are fed tosimpletest_process_phpunit_tests()
in one call.Thanks for the info and links to the other issues. I can see that many major changes are coming for PHPunit and WebTest tests, but it is good to have this exception fixed now, before those other changes come along.
Jonathan