Closed (fixed)
Project:
Search API
Version:
8.x-1.x-dev
Component:
Tests
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
18 Feb 2016 at 15:38 UTC
Updated:
19 Jul 2016 at 08:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
borisson_I know you like merging tests, I prefer having smaller tests, I don't think the time really matters on the testbots, I agree that it can be annoying locally but I never run a full test-class anyway (I comment out the unrelated test methods).
https://www.drupal.org/pift-ci-job/182570

I think its a matter of taste and if you want to do this - it's a perfect novice issue. I'd love more opinions though :)
Comment #3
drunken monkeyThanks for weighing in!
I agree, I'd also like more opinions – but that, unfortunately, rarely happens. But we can wait a bit and see, it's not urgent after all.
In any case, it's really interesting how quick the tests run on the test bot. The times now seem to be more around 1½ minutes, but still amazing. However, no matter how they do it, unless they are running a cleverer version of Simpletest than shipped with Drupal (which I doubt), additional test methods will still put an additional strain on the infrastructure.
Comment #4
jhedstrom+1 for merging methods that extend WebTestBase to reduce testing time.
Comment #5
drunken monkeyOK, then let's do this. Thanks for weighing in!
As suggested by Joris, adding the "Novice" tag.
Comment #6
anicky commentedFirst time for me to modify functional tests in Drupal, so I'm not sure about process and conventions.
I simply replaced
public function testMethodswithprotected function checkMethods, and I created onepublic function testMethodthat merges the fourcheckMethods. Also, I used comments that were inBackendTestBasefor thetestFrameworkmethod as a model for this merging method.Is it correct or am I totally wrong?
Comment #9
anicky commentedSince the four methods was merged into one, there was a problem because the first test logout the user, and the remaining tests need to be logged as admin (which was made at the beginning of each test before).
Comment #12
anicky commentedI forgot to make an interdiff for patch #9, sorry for that.
Comment #13
anicky commentedI had to do a change on
getTestServerandgetTestIndex, because there was an exception saying that the server or the index already exists.Merging tests is fine to reduce time, but it also implies that the different tests are not independent anymore. In this case:
Comment #14
drunken monkeyVery good, thanks!
I tried to get rid of most of those workarounds in the attached patch. Let's see if that still passes.
Comment #17
anicky commentedComment #18
drunken monkeyAh, damn, would have hoped that was only failing locally.
But now that the test methods are merged, there's no reason not to use the normal way of installing the module anyways.
Comment #19
borisson_Comment #21
drunken monkeyThanks for reviewing! Committed.
Thanks a lot again for your work on this, Anicky!