Unlike IntegrationTest, OverviewPageTest currently uses separate methods for its separate tests. While that is of course the recommended and "clean" structure, the inefficiencies of Simpletest mean that this will triple (quadruple soon, with #2659862: Move overview testing code to the overview test.) the test's runtime completely unnecessarily.
I would therefore vote for changing those three/four test methods to helper methods for a new, single, encompassing test method.

Comments

drunken monkey created an issue. See original summary.

borisson_’s picture

Issue summary: View changes
StatusFileSize
new24.7 KB

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 :)

drunken monkey’s picture

Thanks for weighing in!

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 :)

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.

jhedstrom’s picture

+1 for merging methods that extend WebTestBase to reduce testing time.

drunken monkey’s picture

Issue tags: +Novice

OK, then let's do this. Thanks for weighing in!
As suggested by Joris, adding the "Novice" tag.

anicky’s picture

Assigned: Unassigned » anicky
Status: Active » Needs review
Issue tags: +DevDaysMilan
StatusFileSize
new1.8 KB

First time for me to modify functional tests in Drupal, so I'm not sure about process and conventions.

I simply replaced public function testMethods with protected function checkMethods, and I created one public function testMethod that merges the four checkMethods. Also, I used comments that were in BackendTestBase for the testFramework method as a model for this merging method.

Is it correct or am I totally wrong?

Status: Needs review » Needs work

The last submitted patch, 6: merge_overviewpagetest-2671264-6.patch, failed testing.

The last submitted patch, 6: merge_overviewpagetest-2671264-6.patch, failed testing.

anicky’s picture

Status: Needs work » Needs review
StatusFileSize
new1.9 KB

Since 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).

Status: Needs review » Needs work

The last submitted patch, 9: merge_overviewpagetest-2671264-9.patch, failed testing.

The last submitted patch, 9: merge_overviewpagetest-2671264-9.patch, failed testing.

anicky’s picture

I forgot to make an interdiff for patch #9, sorry for that.

anicky’s picture

Status: Needs work » Needs review
StatusFileSize
new3.45 KB
new1.55 KB

I had to do a change on getTestServer and getTestIndex, 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:

  • After the first test, it is necessary to log again as admin user (because the first one checks permissions and log out then)
  • The test server and test index have to be removed before each test (using the same server/index causes problems apparently)
drunken monkey’s picture

Very good, thanks!
I tried to get rid of most of those workarounds in the attached patch. Let's see if that still passes.

Status: Needs review » Needs work

The last submitted patch, 14: 2671264-14--overwiew_page_test_refactoring.patch, failed testing.

The last submitted patch, 14: 2671264-14--overwiew_page_test_refactoring.patch, failed testing.

anicky’s picture

Assigned: anicky » Unassigned
drunken monkey’s picture

Status: Needs work » Needs review
StatusFileSize
new830 bytes
new10.27 KB

Ah, 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.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for reviewing! Committed.
Thanks a lot again for your work on this, Anicky!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.