Problem/Motivation

14 test methods means that the test basically takes as long as 14 separate test classes with a single test method, except that it can't be run in parallel.

Try to group the test methods into 3-4 groups that belong to together, make separate test classes for them. That should speed up test bot, as it can run them in parallel.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

miro_dietiker’s picture

Title: DiffPluginsTest contains 14 test methods, split into multiple classes » DiffPluginTest contains 14 test methods, split into multiple classes
miro_dietiker’s picture

We could also group the test methods as:
function testPlugins() {
doTestCommentPlugin();
doTestCorePlugin();
...
}

This removes most time = setup. And the tests are indeed pretty isolated and don't need a new environment.

BTW each method should contain a @see reference to its corresponding plugin class.

miro_dietiker’s picture

Priority: Normal » Minor

Prio down, although still happy to see this cleanup happen.

Ginovski’s picture

Assigned: Unassigned » Ginovski
Status: Active » Needs review

Added method and @see references to the plugins.

Ginovski’s picture

Status: Needs review » Needs work

The last submitted patch, 6: diffplugintest_contains-2822537-5.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review
FileSize
4.17 KB
883 bytes

Moving the failing one into a separate test again.

Status: Needs review » Needs work

The last submitted patch, 8: diff_2822537_plugin_8.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review
FileSize
4.11 KB
1.61 KB

  • miro_dietiker committed caff541 on 8.x-1.x
    Issue #2822537 by miro_dietiker, Ginovski: DiffPluginTest contains 14...
miro_dietiker’s picture

Status: Needs review » Needs work

Committing this temporary improvement.

Strange: I have lots of failing tests locally. Didn't figure out yet why.
Back to needs work to to apply the pattern to the other ones that failed.

johnchque’s picture

The test will fail locally if you have the Visual Inline libraries installed.

miro_dietiker’s picture

Created issue to clean-up / fix test fails related to htmldiff: #2823903: Make tests pass when html diff library is present

Keeping this issue needs-work to further split the tests / reduce test methods in DiffPluginTest.

Ginovski’s picture

Status: Needs work » Needs review
FileSize
16.96 KB

1. Added new DiffEntityPluginTest with 3 methods extracted from DiffPluginTest
-testEntityReferencePlugin
-testFilePlugin
-testImagePlugin
2. Configured DiffPluginTest and removed default configs that weren't needed
Tests passed locally

Ginovski’s picture

Small fix in the DiffPluginTest, added description and deleted extra line.

miro_dietiker’s picture

+++ b/src/Tests/DiffEntityPluginTest.php
@@ -0,0 +1,268 @@
+class DiffEntityPluginTest extends DiffTestBase {
...
+  public function testEntityReferencePlugin() {
...
+  public function testFilePlugin() {
...
+  public function testImagePlugin() {

That's now 3 cases in newly created class.
@Berdir, did you intend this or a test class per plugin, or stay with the current approach and convert them to a doTestXYZ pattern because they don't necessarily need a full reinstall - except that they currently accidentally clash?

miro_dietiker’s picture

FileSize
34.86 KB

Shifted some stuff around. The PluginTest still needs division. But all other tests now use variables for all ID's and made the views configuration optional. Their runtime is much lower.

miro_dietiker’s picture

FileSize
50.76 KB

And now with all the things separated.
Thought i would do more test classes - one per plugin - but that was not needed as the plugins can be tested with doXYZ approach easily.
Test run time down nicely. Just below 1m30 for me.

Status: Needs review » Needs work

The last submitted patch, 19: diff_2822537_plugin_19.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review
FileSize
67.24 KB

Retryyyy..

Status: Needs review » Needs work

The last submitted patch, 21: diff_2822537_plugin_20.patch, failed testing.

  • miro_dietiker committed 4e26fe0 on 8.x-1.x
    Issue #2822537 by miro_dietiker, Ginovski: DiffPluginTest contains 14...
miro_dietiker’s picture

Status: Needs work » Fixed

Oh... Committed with declaring the base class abstract. Yippie! :-)

Status: Fixed » Closed (fixed)

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