Problem/Motivation

Using @expectedException is not considered good testing practice because you cannot specify where exactly in the test method the exception should be triggered. Background info: https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

Proposed resolution

We should use the method setExpectedException() instead right before the line that triggers the exception (usually right before the last line).

Therefore @martin107 and @vaplas worked on an automated conversion script: https://github.com/martinfrances107/convertExceptions

Remaining tasks

Review patch.

CommentFileSizeAuthor
#109 interdiff.txt486 bytesvaplas
#109 2822837-109.patch188.83 KBvaplas
#107 2822837-107.patch188.35 KBvaplas
#101 interdiff.txt632 bytesvaplas
#101 2822837-101.patch189.78 KBvaplas
#99 2822837-99.patch189.47 KBvaplas
#99 interdiff.txt1.29 KBvaplas
#96 interdiff-2822837-86-90.txt157.64 KBchiranjeeb2410
#92 interdiff.txt6.39 KBvaplas
#92 2822837-92.patch189.78 KBvaplas
#90 2822837-90.patch4.42 KBchiranjeeb2410
#86 interdiff.txt8.11 KBvaplas
#86 2822837-86.patch189.02 KBvaplas
#85 interdiff.txt913 bytesvaplas
#85 2822837-85.patch188.11 KBvaplas
#82 interdiff.txt1.6 KBvaplas
#82 2822837-82.patch188.1 KBvaplas
#79 interdiff-8.3-8.4.txt1.09 KBvaplas
#79 2822837-79-8.4.patch187.98 KBvaplas
#78 interdiff.txt1.69 KBvaplas
#78 2822837-78.patch189.07 KBvaplas
#74 bad-test__FormBuilderTest.patch2.33 KBvaplas
#74 bad-test__all-no-last-cases.patch15.95 KBvaplas
#74 2822837-74.patch189.04 KBvaplas
#3 exception-2822837-3.patch3.87 KBmartin107
#7 DO-NOT_COMMIT-2822837-7.patch2.58 KBmartin107
#9 halfMonster.patch133.91 KBmartin107
#11 myMonster.patch159.29 KBvaplas
#13 monster2.patch159.78 KBvaplas
#16 unstoppable_monster.patch159.78 KBvaplas
#18 green_monster.patch159.81 KBvaplas
#19 convertExpectedException.php_.txt3.74 KBvaplas
#19 interdiff-7-18.txt4.13 KBvaplas
#25 prettyMonster-2822837-25.patch157.49 KBmartin107
#26 blondyMonster-2822837-26.patch177.07 KBvaplas
#26 ExpectedExceptionMessageRegExp_miniMonster.patch.txt2.44 KBvaplas
#26 ConfigTest_testValidateNameException_tailMonster.patch.txt546 bytesvaplas
#28 chiterMonster-2822837-28.patch177.21 KBvaplas
#28 interdiff_Blonde-Chiter_26-28.txt5.52 KBvaplas
#33 makeupMonster-2822837-33.patch180 KBvaplas
#33 interdiff_makeup_chiter-28-33.txt31.88 KBvaplas
#35 makeupMonster-2822837-35.patch179.96 KBvaplas
#37 makeupMonster-2822837-37.patch179.95 KBvaplas
#40 script.zip530.09 KBvaplas
#42 2822837-42.patch185.44 KBvaplas
#44 victorianMonster.patch187.75 KBvaplas
#44 violatorMonster.patch184.76 KBvaplas
#48 2822837-48.patch187.51 KBvaplas
#49 2822837-49-only-fails.patch184.52 KBvaplas
#51 manual_fix_request_exception-2822837-51-only-fails.patch184.56 KBvaplas
#53 red_dwarf_2822837-53-fails-only.patch19.1 KBvaplas
#57 2822837-57.patch187.54 KBvaplas
#58 interdiff-first_line-algo_line.txt131.85 KBvaplas
#65 2822837-65.patch187.66 KBvaplas
#65 interdiff-57-65.txt4.1 KBvaplas
#65 script.php_.txt16.38 KBvaplas
#70 prepare_patch_2822837-26.patch.txt2.97 KBvaplas
#70 script.php_.txt16.4 KBvaplas
#70 main-interdiff-between-patches-65-70.txt5.62 KBvaplas
#70 2822837-70.patch189.46 KBvaplas
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

martin107 created an issue. See original summary.

martin107’s picture

Issue summary: View changes
martin107’s picture

Assigned: martin107 » Unassigned
Status: Active » Needs review
FileSize
3.87 KB
dawehner’s picture

Instead of doing that in one single test we should ideally do that across core. Even better would be of course if we would somehow script that particular change.

martin107’s picture

I have lots of sympathy with that attitude

When I say over 1000 in the issue summary, it is because I 'grep'ed around in my IDE.

so ... It is going to have to be a script - that does a careful bit of string manipulation.

It is tricky because... even if we break that monster patch into 100 sub issues it is still a big ask for a reviewer to visually check.

So I guess we will work this out some way over the next 5 years.

PS

This patch changes only one file ...but at the time I wrote it - I made sure it covered the whole asset library subsystem.

dawehner’s picture

that does a careful bit of string manipulation.

Well, with libraries like https://github.com/grom358/pharborist we could build automatic rewrite tools.

It is tricky because... even if we break that monster patch into 100 sub issues it is still a big ask for a reviewer to visually check.

Well, which is basically exactly what you do here :) Over time each individual file will be converted and requires full new attention by the review person/committer. If you have just one patch with one logic change, you just need a single of those attentions, which might be huge, but its still smaller than the sum of a lot of microattentions.

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs review » Active
FileSize
2.58 KB

Ok so my first pharborist conversion routine.

It is 70% complete. - the conversion script is posted on github

@dawehner could I ask you to briefly have a scan over it

https://github.com/martinfrances107/convertExceptions/blob/master/conver...

As I only started looking at pharborist today it strikes me that I could be ignoring well all the established high performance patterns of doing things - I could have created something so crappy that it needs rewriting

so the pseudo-code


iterate of over all *Test.php file in core
find every doc-block ( ie starting with /** )
if found extract all @expectedException, and @expectedExceptionMessage
and insert a new "$this->setExpectedException()" command into the start of the method below the doc-block

I have yet to implement

insert a new "$this->setExpectedException()" command into the start of the method below the doc-block

I would be grateful if you could take a brief look and tell me in broad terms If I am on the right track

.... more tomorrow

PS

It looks like I have exposed a brittleness in pharborist - the iteration over core files only works if the following flaws are fixed in core ( see DO-NOT_COMMIT-2822837-7.patch )

for the moment I am getting it to work on a sample test class called test.php

dawehner’s picture

That is amazing!! Thank you for trying to solve things properly. To be honest I just talked in general terms and did not expected that you actually write the script.

martin107’s picture

Component: asset library system » phpunit
Status: Active » Needs review
FileSize
133.91 KB

Well, as usual I seem to only be able to complete half the remaining tasks :)

I can see a few flaws :-
the indentation is still not correct.
I have yet to remove the @expectedException and @expcectedMessage tags

We spoke of a monster patch - well this is a "half monster" - I just want to see what testbot will say

more soon.

Status: Needs review » Needs work

The last submitted patch, 9: halfMonster.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
159.29 KB

@martin107 - nice script! I'm bit changed it. But first - what you think about it, Mr. Bot?)

Status: Needs review » Needs work

The last submitted patch, 11: myMonster.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
159.78 KB

Come on, Bot!)

Status: Needs review » Needs work

The last submitted patch, 13: monster2.patch, failed testing.

vaplas’s picture

Okey. In my first patch was not correctly placed quotes. But the second patch also failed. What the problem now? Maybe the bot just does not like my monsters?( See you again, Bot. Sorry for spam.

vaplas’s picture

Status: Needs work » Needs review
FileSize
159.78 KB

Status: Needs review » Needs work

The last submitted patch, 16: unstoppable_monster.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
159.81 KB
vaplas’s picture

It alive!)

martin107’s picture

Status: Needs review » Needs work

The monster lives!

@vaplas thank you for moving the script on - it is good to see that yours is much more complete than mine.

Just making notes as I visually scan the patch - it looks like the corner cases are MOSTLY being handled correctly.

1)
File '/non/existent/file' does not exist
becomes
'File \'/non/existent/file\' does not exist'

2)
requires a value for the "$route" argument.
becomes
'requires a value for the "$route" argument.'

3)
Link template 'canonical' for entity type 'apple' must start with a leading slash, the current link template is 'path/to/apple'
becomes
'Link template \'canonical\' for entity type \'apple\' must start with a leading slash, the current link template is \'path/to/apple\''

4)
'requires a value for the "$route" argument.'
becomes
'requires a value for the "$route" argument.'

Which is all good.

BUT - thing go wrong in

/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php

is looks like using # as a string delimiter is causing problems these lines

- * @expectedException \Drupal\Core\Extension\InfoParserException
- * @expectedExceptionMessageRegExp #broken\.info\.txt#

are exchanged with
+ $this->setExpectedException('\Drupal\Core\Extension\InfoParserException');
+

so the message is dropped. - I am working on this now

vaplas’s picture

@martin107, great points! Thank you!

About my mistakes (after refactoring):

1. for pretty message with ' and " we can use something like this:

      if ($message_tags) {
        $message = $message_tags[0]->getDescription();
        if(strpos($message, "'") !== FALSE) {
          if(strpos($message, '"') !== FALSE) {
            $message = preg_replace('/"/', '\\"', $message);
          }
          $message = ', "' . $message . '"';
        }
        else{
          $message = ", '" . $message . "'";
        }
      }
      else {
        $message = '';
      }

# ....

      $statement = "\$this->setExpectedException('" . $exception_class . "'"  . $message . ");" . PHP_EOL . PHP_EOL. $indentation;

#result

"The source is frozen and can't be changed any more"
"File '/non/existent/file' does not exist"
'requires a value for the "$route" argument.'
"Text \"can't breaks!\" text"

2. $method_class->setDocComment($doc_comment); add new line, i don't know why? But we can fix it like:

      $result = $tree->getText();
      $result = preg_replace('@\*/\n  (?=\n)@', '*/', $result);
      file_put_contents($filename, $result);

3. Problem with two different variant Message Tag, not # delimiter. But i don't know what means "RegExp" in this context

      $message_tags = $doc_block->getTagsByName('expectedExceptionMessage');
      if(empty($message_tags)){
        $message_tags = $doc_block->getTagsByName('expectedExceptionMessageRegExp');
      }

Edit: I looked all places, we have only "expectedExceptionMessage" and "expectedExceptionMessageRegExp" (only one file! how did you find it?!).

martin107’s picture

(1)
Ah I was just making notes as I went along, and trying to develop a good mental map of the things that could go wrong. I was not suggesting that we fix it up ... although the output of your changes does look good.

I did not notice (2) - I will work that into the next script - thank you

only one file! how did you find it?!

We are about to ask that a core committer sits down with a cup of coffee and review the patch .. he/she is going to have a headache at the end.
So when I say "I did a visual scan of the patch" well it is something that takes time :)

I am being pulled away onto something else ... I will work on this over the weekend.

PS regarding your script from #19

I was feeling grumpy that I did not know how to fix the indentation problems I was having I see that you fixed so much more ....but your solution to that is simple elegance..

vaplas++

I have pushed your code into github (and given you credit. )

Thanks again

vaplas’s picture

It's great that you have accepted the challenge and strongly want to complete this issue. I am pleased to help you!

dawehner’s picture

You two are my heroes! Its serious a nice work.

One thing which would be really nice, but is probably quite hard: Move the setExpectedException call, nearer/next to the actual called/tested function. We could look at the @covers annotation and in case there is one, move the setExpectedException just the line before that.

martin107’s picture

Status: Needs work » Needs review
Related issues: +#2824989: Refactor @expectedExceptionMessageRegExp as a special case.
FileSize
157.49 KB

From #21

1) Pretty print enabled.

2) Now filtering out the second line

3) Its a time management thing ... I want to fix these 3 special cases manually rather than update the script. So I have created a side issue.

From #24 .. thank you :)

One thing which would be really nice, but is probably quite hard: Move the setExpectedException call, nearer/next to the actual called/tested function

I am still thinking about that ... there are plenty of examples such as

FileCopyTest::testNonExistentSourceFile()

where as you say we are going to have to fall back to the existing insert at the top code.

PS the script has a flaw

/core/tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php

There are some newlines added at the end of the modified doc block

I have pushed all of vaplas work onto github, so the current up to date script is found at

https://github.com/martinfrances107/convertExceptions

( With attribution in the commit log )

vaplas’s picture

24: Crazy nice to hear this from the man whose commits enough to build Drupal from scratch!
25: Quality work as always!

But it may better to collect all the monsters one place?

For example, I also found a monster (tail of monster) in ConfigTest.php

  /**
   * @covers ::validateName
   * @expectedException \Drupal\Core\Config\ConfigNameException !TAIL!
   * @dataProvider validateNameProvider
   */
  public function testValidateNameException($name, $exception_message) {
    $this->setExpectedException('\Drupal\Core\Config\ConfigNameException', $exception_message);

And now: fix new line + added your miniMonster + my tailMonster + the first attempt to add brains to our monster)

Status: Needs review » Needs work

The last submitted patch, 26: blondyMonster-2822837-26.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
177.21 KB
5.52 KB

Ogo! 6/233 places (97 IQ). Not bad. But now I'm blonde check the optimality of his choices).. And bit chit just for green.

dawehner’s picture

You continue to rock. One thing which would make it even better:

$this->setExpectedException('\Drupal\migrate\MigrateException');

this could also be

use Drupal\migrate\MigrationException;

$this->setExpectedException(MigrationException::class);
martin107’s picture

thanks vaplas

We could look at the @covers annotation and in case there is one, move the setExpectedException just the line before that.

I have pushed a improved algorithm to

https://github.com/martinfrances107/convertExceptions

It seems to work on an improved test.php

if there are multiple calls to bar then I also say we should fall back to inserting new code at the top

for example

/**
*
* @covers ::bar
*/
testZYZ() {
  $a = 1;
  $Foo::bar();
  $a++;
  $Foo::bar();
}

I will work on this some more sunday ..

Status: Needs review » Needs work

The last submitted patch, 28: chiterMonster-2822837-28.patch, failed testing.

dawehner’s picture

Title: LibraryDiscoveryParserTest @expectedException @expectedExceptionMessage are deprecated » Replace @expectedException @expectedExceptionMessage with $this->setExpectedException

Let's make the title clearer what you two try to do.

vaplas’s picture

Status: Needs work » Needs review
FileSize
180 KB
31.88 KB

Status: Needs review » Needs work

The last submitted patch, 33: makeupMonster-2822837-33.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
179.96 KB

Mr. Bot you are nudnik! It is hard to fix yourself? Because of you, I got caught to copy-paste). Quick manual correction.

Status: Needs review » Needs work

The last submitted patch, 35: makeupMonster-2822837-35.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
179.95 KB

I think we are well popularize the setExpectedException practice, due to my ignorance)

vaplas’s picture

Changes from #28:

  1. replace "manual correct" on "auto correct by manual map" (only for 2 cases, see lines with "ALGO MANUAL MAP")
  2. 1 line up for all assert algo case (and 2 line, if prev node == $expected) (see lines with "ALGO ASSERT")
  3. MigrateException::class done (see lines with "MigrateException::class")
  4. relocation outside loops (for, foreach, while, do while cases) (see line with "a/core/tests/Drupal/Tests/Component/Utility/RandomTest.php"
  5. delete duplicate * *

@martin107, I changed the code a bit. And I need some time to сlear it. But if you want, I can attach dirty version now.

martin107’s picture

But if you want, I can attach dirty version now.

That would be great.

I have a partial fragment, but it is at a very early stage.

I commented on this pharborist issue, if we can, I would like to find a solution that can be pushed upstream.

https://github.com/grom358/pharborist/issues/144

vaplas’s picture

FileSize
530.09 KB

Yes, it would be great to improve the library pharborist!
I lost a lot of time searching for a beautiful way to add a declaration with the current API, but failed. And I made it very crookedly. There are other unpleasant moments, but maybe I just is not undestand it. Example, not detected method on deep level:

# @covers: sendMailMessages
$this->contactMailHandler->sendMailMessages($message, $sender);

My last patch have typo ('foreach' not found). Script in attached archive fix it.

I was starting to refactor and much stepped aside :( But I hope you can extract necessary points and unite with your work!

martin107’s picture

Assigned: martin107 » Unassigned

@vaplas

thanks, I really like to way things are broken up into manageable functions calls so at the top level this is appropriate.

      $line = lineDetector($method_class, $doc_block, $filename);

      $new_node = createExceptionNode($class_tags, $doc_block, $method_class, $line, $tree);
      $new_node->insertBefore($line);

      $doc_comment = clearDocComment($method_class);
      $method_class->setDocComment($doc_comment);

      $result = removeArtefacts($tree);

I am going to be busy next week so my time for open source is limited.

As far as thing go

I think there is a bug in Pahrborist

in the area of RootNode::getUseDeclarations() for which I need to file a bug.

That needs fixing before, the ensure()/import() method from

https://github.com/grom358/pharborist/issues/144

can be implemented [ which would give us the chance to "ensure" that a use statement is present in a single command]

I see you have a lot of time and energy to put into this, and I hope we can coordinate our activities , I will help out where possible, but I think I should unassign myself.

If you want to do more here, I will be around to provide helpful reviews.

vaplas’s picture

FileSize
185.44 KB

Status: Needs review » Needs work

The last submitted patch, 42: 2822837-42.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
187.75 KB
184.76 KB

Friday - monsters time!

1. Fix problem with several namespaces in one file;
2. Fix problem with adding 'use declaration' in alphabetical order;
3. violatorMonster - experiment how many fails will be if shift position in one line down.

The last submitted patch, 44: victorianMonster.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 44: violatorMonster.patch, failed testing.

The last submitted patch, 44: victorianMonster.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
187.51 KB
vaplas’s picture

Sorry for #44, week without monsters affected :(
Continuation of the experiment

Status: Needs review » Needs work

The last submitted patch, 49: 2822837-49-only-fails.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
184.56 KB

I do not understand #49 CI error very well, but I hope this manual change will fix it.

Status: Needs review » Needs work

The last submitted patch, 51: manual_fix_request_exception-2822837-51-only-fails.patch, failed testing.

vaplas’s picture

vaplas’s picture

Status: Needs work » Needs review

The last submitted patch, 7: DO-NOT_COMMIT-2822837-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 53: red_dwarf_2822837-53-fails-only.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
187.54 KB

24 fails? But patch has only 22 setExpectedException :)

Ok. If I understand correctly, this patch must have all calls setExpectedException near the problem places (#24)

vaplas’s picture

At last!) See different between first line and line by algo in attached file.

if there are multiple calls to bar then I also say we should fall back to inserting new code at the top

@martin107, I understand your desire to add heuristics, but it's arguable point for me. And maybe we don't need in artificial intelligence.

Now patch #57 injects all calls near a first problem places, and someone can use of this fact, when will manual solves this issue. I think this is what we need.

Please, suggestions for further work.

And my questions:

1. Does anyone know of a good algorithm of insertion 'use declaration'? Alphabet paste seems not always successful, for example:

#example 1
   use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
   use Symfony\Component\DependencyInjection\Reference;
   use Symfony\Component\DependencyInjection\Parameter;
   use Symfony\Component\ExpressionLanguage\Expression;

#example 2
 use Drupal\Component\Plugin\Definition\PluginDefinitionInterface;
+use Drupal\Component\Plugin\Exception\PluginException;
 use Drupal\Component\Plugin\Factory\DefaultFactory;
 use Drupal\plugin_test\Plugin\plugin_test\fruit\Cherry;
 use Drupal\plugin_test\Plugin\plugin_test\fruit\FruitInterface;

2. What better use

+    $this->setExpectedException(\Exception::class);

or

+use Exception;
..
+$this->setExpectedException(Exception::class);

Thanks!

dawehner’s picture

Great work! I think this is groundbreaking work for further automatic rewrites of Drupal potentially!

@martin107, I understand your desire to add heuristics, but it's arguable point for me. And maybe we don't need in artificial intelligence.

Now patch #57 injects all calls near a first problem places, and someone can use of this fact, when will manual solves this issue. I think this is what we need.

One strategy we could apply is to sort of place them okay, and exclude files, where this script doesn't work as expected. These excluded files could then be converted in a follow up manually. Does that make sense?

What better use

+ $this->setExpectedException(\Exception::class);
or

+use Exception;
..
+$this->setExpectedException(Exception::class);
Thanks!

We are using the first one.

klausi’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs change record

Wow, awesome work here! The patch looks almost perfect.

Tiny nitpick:

+++ b/core/tests/Drupal/Tests/Core/Render/RendererRecursionTest.php
@@ -41,7 +39,8 @@ class RendererRecursionTest extends RendererTestBase {
     $complex_child = $complex_child_template;
     $callable = function () use ($renderer, $complex_child) {
-      $renderer->renderRoot($complex_child);
+      $this->setExpectedException(\LogicException::class);
+    $renderer->renderRoot($complex_child);
     };

indentation error

We should also have a change record to inform people to use setExpectedException in the future.

martin107’s picture

I am happy to see this issue moving on

Here is a draft change record.

https://www.drupal.org/node/2827160

dawehner’s picture

@martin107 @vaplas
I just try to understand that a bit better. Is the entire patch autogenerated?

vaplas’s picture

Many thanks for pretty words and helps!!!

Tiny nitpick:

great catch! I'll fix algorithm indentation with next patch

Is the entire patch autogenerated?

It autogenerated, but with several manual rules for few special cases + patches.txt from #26). In reality, I think we can simplify the script to the madness, because the vast majority of cases - it is the last line, and we do not need all of that logic, that i stuffed :( I will try to carry out this simplification (saved the current script only for the history).

martin107’s picture

Issue tags: -Needs change record
vaplas’s picture

Status: Needs work » Needs review
FileSize
187.66 KB
4.1 KB
16.38 KB
  • fix indentation error (#60)
  • a bit improved insertion 'use declaration' (via attempt to use the delimiter '\' between names)
  • script for history (I could not simplify it, sorry)

Quick Guide by script:

  1. apply patches.txt from #26
  2. setting $PATH and run script
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Amazing work!

Wim Leers’s picture

This is EPIC! Incredible work!

catch’s picture

Status: Reviewed & tested by the community » Postponed

This looks great, but we should either commit it to both 8.2.x and 8.3.x, or commit it to 8.3.x only around beta/RC. Committing it only to 8.3.x now means more chance of patches not cherry-picking.

We normally do the latter for big scripted changes like this, so marking postponed.

If I'm missing something and this should go into 8.3.x and 8.2.x now then please re-RTBC so we can discuss more though.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vaplas’s picture

Status: Needs review » Postponed

OK.

vaplas’s picture

It is correct time for unpostponed?

xjm’s picture

Version: 8.4.x-dev » 8.3.x-dev
Issue tags: +rc target

@tim.plunkett pointed this out to me. From #68, it looks like @catch intended this to be an RC target.

This is likely to share context lines with #2854529: Fix Drupal.Scope.MethodScope - all methods should have scopes, so let's get that one in first, and then unpostpone this one. Thanks!

vaplas’s picture

@tim.plunkett, @xjm, thank you very mach!

To facilitate verification, I performed the action like #53 (see bad-test__* patches). That is added code:

    if ($index_line == count($lines)-1) {
      return null;
    }
    else {
      $index_line++;
    }

i.e. increases all positions by one line (insert $this->setExpectedException( after hypothetical exception), and save only cases where line != last line (because it is no need to check). Separate patch for FormBuilderTest because CI error like #49. Bad patches should fail in all cases if all right.

In main patch after script add manual fix indentations:

+++ b/core/tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php
-use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
-use Symfony\Component\DependencyInjection\Exception\RuntimeException;
+  use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+  use Symfony\Component\DependencyInjection\Exception\RuntimeException;

If all right, I run a script to 8.4.x

The last submitted patch, 74: bad-test__all-no-last-cases.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 74: bad-test__FormBuilderTest.patch, failed testing.

klausi’s picture

Nice work!

  1. +++ b/core/modules/migrate/tests/src/Unit/RowTest.php
    @@ -73,24 +73,22 @@ public function testRowWithMultipleSourceIds() {
       /**
        * Tests source immutability after freeze.
    -   *
    -   * @expectedException \Exception
        */
       public function testSourceFreeze() {
         $row = new Row($this->testValues, $this->testSourceIds);
         $row->rehash();
    +    $this->setExpectedException(\Exception::class);
         $this->assertSame($this->testHash, $row->getHash(), 'Correct hash.');
         $row->setSourceProperty('title', 'new title');
         $row->rehash();
    

    is this correct? why is the setExpectedException() call not further down?

  2. +++ b/core/tests/Drupal/Tests/Component/Plugin/Discovery/DiscoveryTraitTest.php
    @@ -58,7 +59,6 @@ public function providerDoGetDefinitionException() {
    @@ -69,6 +69,7 @@ public function testDoGetDefinitionException($expected, $definitions, $plugin_id
    
    @@ -69,6 +69,7 @@ public function testDoGetDefinitionException($expected, $definitions, $plugin_id
         $method_ref = new \ReflectionMethod($trait, 'doGetDefinition');
         $method_ref->setAccessible(TRUE);
         // Call doGetDefinition, with $exception_on_invalid always TRUE.
    +    $this->setExpectedException(PluginNotFoundException::class);
         $this->assertSame(
           $expected,
           $method_ref->invoke($trait, $definitions, $plugin_id, TRUE)
    

    It doesn't really make sense to use assertSame() here. The @dataProvider on that method also does not make sense because it has only one test set. Anyway, that is out of scope for this issue, so the change is fine as is.

  3. +++ b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
    @@ -131,6 +128,7 @@ public function testGetWildcardArgumentNoTypehint() {
         $callable = function($route) {};
    +    $this->setExpectedException(\RuntimeException::class, 'requires a value for the "$route" argument.');
         $arguments = $resolver->getArguments($callable);
         $this->assertNull($arguments);
    

    Same here: assertNull() is useless, $arguments is useless.

  4. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbTest.php
    @@ -15,11 +15,10 @@ class BreadcrumbTest extends UnitTestCase {
       /**
        * @covers ::setLinks
    -   * @expectedException \LogicException
    -   * @expectedExceptionMessage Once breadcrumb links are set, only additional breadcrumb links can be added.
        */
       public function testSetLinks() {
         $breadcrumb = new Breadcrumb();
    +    $this->setExpectedException(\LogicException::class, 'Once breadcrumb links are set, only additional breadcrumb links can be added.');
         $breadcrumb->setLinks([new Link('Home', Url::fromRoute('<front>'))]);
         $breadcrumb->setLinks([new Link('None', Url::fromRoute('<none>'))]);
       }
    

    This does not look correct - where should the exception be thrown? On the last line or the line before? If it is the line before then the last line is useless and should be removed.

    Yay, finding bugs in test code with this conversion!

vaplas’s picture

Status: Needs work » Needs review
FileSize
189.07 KB
1.69 KB

@klausi, great points!

1, 4 - Mr. Bot also also found no problems with a shift down BreadcrumbTest and RowTest - https://www.drupal.org/pift-ci-job/615076. Strangely enough, I definitely remember that I was putting the rules in this was necessary for \Drupal\Tests\migrate\Unit\RowTest::testSourceFreeze :) All other tests have fails (with the exception of RandomTest.php, but it ok, because has for() and possition corrected).

2, 3 - valuable findings! New issue, right?

vaplas’s picture

Looks like we have only one different case with patch for 8.4. It just hasn't /core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php.

The last submitted patch, 78: 2822837-78.patch, failed testing.

vaplas’s picture

oops, my mistake. Branches haven't difference. I'm just overlooking #2852242: Merge the two files testing the file_copy process plugin. Hence #78 patch Failed to Apply. And #79 suitable for both branches.

77.2 - #2858325: Clean up DiscoveryTraitTest::testDoGetDefinitionException
77.3 - #2858327: Clean up BreadcrumbTest::testSetLinks

PS. Just for clarity, I was also mistaken when I spoke about CI error with FormBuilderTest, if someone is trying to read #74 :)

vaplas’s picture

Trying to fix 3 coding standards messages with #79 patch.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextDefinitionTest.php
    @@ -107,7 +107,6 @@ public function testGetDataDefinition($is_multiple) {
    @@ -155,6 +154,7 @@ public function testGetDataDefinitionInvalidType($is_multiple) {
    
    @@ -155,6 +154,7 @@ public function testGetDataDefinitionInvalidType($is_multiple) {
           ->method('getDataType')
           ->willReturn($data_type);
     
    +    $this->setExpectedException(\Exception::class);
         $this->assertSame(
           $mock_data_definition,
           $mock_context_definition->getDataDefinition()
    

    Another instance where the assertSame() should be removed in a follow-up.

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
    @@ -87,8 +87,6 @@ public function testMatchRequestAllowed() {
    @@ -98,6 +96,7 @@ public function testMatchRequestDenied() {
    
    @@ -98,6 +96,7 @@ public function testMatchRequestDenied() {
           ->method('checkRequest')
           ->with($request)
           ->willReturn($access_result);
    +    $this->setExpectedException(AccessDeniedHttpException::class);
         $parameters = $this->router->matchRequest($request);
         $expected = [
           AccessAwareRouterInterface::ACCESS_RESULT => $access_result,
    

    This should probably have another follow-up. The $expected line will never be reached, right?

  3. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslatableMarkupTest.php
    @@ -82,24 +82,20 @@ public function testToString() {
       public function testIsStringAssertionWithFormattableMarkup() {
         $translation = $this->getStringTranslationStub();
    +    $this->setExpectedException(\InvalidArgumentException::class, '$string ("foo") must be a string.');
         $formattable_string = new FormattableMarkup('@bar', ['@bar' => 'foo']);
         new TranslatableMarkup($formattable_string);
    

    This change is wrong, should be on the line right before the last line?

I think my last point is a blocker, otherwise looks good!

There are lots of assert*() function calls that will never be reached because the test case throws the exception before that. But that is not the fault of this issue and we will have to clean that up in follow-ups.

klausi’s picture

Opened #2859286: Forbid @expectedException @expectedExceptionMessage for Coder to forbid those 2 doc annotations in our coding standards.

vaplas’s picture

Status: Needs work » Needs review
FileSize
188.11 KB
913 bytes

83.3 - nice catch! Done + remove unused variable.

#84 <3

vaplas’s picture

There are lots of assert*() function calls that will never be reached. But that is not the fault of this issue

Sounds like fix of this issue сould be bit better :) I changed the script and now it is trying to automatically get rid of all unnecessary assert*.

klausi’s picture

+++ b/core/modules/system/tests/modules/accept_header_routing_test/tests/Unit/AcceptHeaderMatcherTest.php
@@ -103,6 +101,7 @@ public function testNoRouteFound() {
     $request->setRequestFormat('json');
     $this->matcher->filter($routes, $request);
     $this->matcher->filter($routes, $request);
+    $this->setExpectedException(NotAcceptableHttpException::class, 'No route found for the specified formats application/json text/xml.');
     $this->fail('No exception was thrown.');

How is this possible? the last line will not throw the exception?

Lol, lol, lol ... congratulations, you found another bug in our test system. This test is never executed! Just executed this test locally in its original form and it is failing (../vendor/bin/phpunit modules/system/tests/modules/accept_header_routing_test/tests/Unit/)

I will open a separate issue for that.

klausi’s picture

klausi’s picture

Status: Needs review » Needs work

Cool, then let's fix all the uncalled asserts() after the exceptions here.

  1. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/ContainerBuilderTest.php
    @@ -35,43 +35,39 @@ public function testSet() {
         $container = new ContainerBuilder();
         $class = new BarClass();
    +    $this->setExpectedException(\InvalidArgumentException::class, 'Service ID names must be lowercase: Bar');
         $container->set('Bar', $class);
         $this->assertNotEquals('bar', $class->_serviceId);
    

    Another instance of a useless assert() after the exception.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php
    @@ -439,14 +441,12 @@ public function testSaveContentEntity() {
         $this->setUpKeyValueEntityStorage();
     
         $entity = $this->getMockEntity('Drupal\Core\Config\Entity\ConfigEntityBase');
    +    $this->setExpectedException(EntityMalformedException::class, 'The entity does not have an ID.');
         $this->entityStorage->save($entity);
         $this->keyValueStore->expects($this->never())
           ->method('has');
    

    Same here, the expects() calls below are never invoked. Which means the exepcts() must be moved before the save() call here.

  3. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
    @@ -126,6 +124,7 @@ public function testDestinationRedirectToExternalUrl($request, $expected) {
         $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
         $dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
         $event = new FilterResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
    +    $this->setExpectedException(\PHPUnit_Framework_Error::class);
         $dispatcher->dispatch(KernelEvents::RESPONSE, $event);
     
         $this->assertEquals(400, $event->getResponse()->getStatusCode());
    

    The assertEquals is never called and should be removed.

  4. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
    @@ -177,6 +174,7 @@ public function testDestinationRedirectWithInvalidUrl(Request $request) {
         $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
         $dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
         $event = new FilterResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
    +    $this->setExpectedException(\PHPUnit_Framework_Error::class);
         $dispatcher->dispatch(KernelEvents::RESPONSE, $event);
     
         $this->assertEquals(400, $event->getResponse()->getStatusCode());
    

    same here.

  5. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -52,15 +53,13 @@ protected function setUp() {
         $form_arg = 'foo';
     
         $clean_form_state = new FormState();
         $form_state = new FormState();
    +    $this->setExpectedException(\InvalidArgumentException::class, 'The form argument foo is not a valid form.');
         $form_id = $this->formBuilder->getFormId($form_arg, $form_state);
     
         $this->assertSame($form_arg, $form_id);
    

    and another useless assertSame().

  6. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -218,14 +217,12 @@ public function testHandleRedirectWithResponse() {
       public function testGetFormWithString() {
         $form_id = 'test_form_id';
         $expected_form = $form_id();
     
    +    $this->setExpectedException(\InvalidArgumentException::class, 'The form argument test_form_id is not a valid form.');
         $form = $this->formBuilder->getForm($form_id);
         $this->assertFormElement($expected_form, $form, 'test');
         $this->assertSame('test-form-id', $form['#id']);
    

    2 useless assert() calls after the exception

  7. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -262,14 +259,12 @@ public function testGetFormWithClassString() {
         $form_id = 'test_form_id';
         $expected_form = $form_id();
     
    +    $this->setExpectedException(\InvalidArgumentException::class, 'The form argument test_form_id is not a valid form.');
         $form = $this->formBuilder->getForm($form_id);
         $this->assertFormElement($expected_form, $form, 'test');
         $this->assertArrayHasKey('#id', $form);
    

    more!

  8. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php
    @@ -158,10 +158,9 @@ public function testIsCached($cache) {
    +    $this->setExpectedException(\LogicException::class);
         $this->decoratedFormState->setCached($cache)
           ->willThrow(\LogicException::class);
     
    

    the exception should be thrown after preparing the mock, so should be moved after it. Also there is an assertSame() following which again will never be called.

  9. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
    @@ -98,6 +96,7 @@ public function testMatchRequestDenied() {
           ->method('checkRequest')
           ->with($request)
           ->willReturn($access_result);
    +    $this->setExpectedException(AccessDeniedHttpException::class);
         $parameters = $this->router->matchRequest($request);
         $expected = [
           AccessAwareRouterInterface::ACCESS_RESULT => $access_result,
    

    $expected will never be called, right? Also $parameters is unused.

chiranjeeb2410’s picture

Assigned: Unassigned » chiranjeeb2410
FileSize
4.42 KB

@klausi,

Uploading latest changes mentioned as in #89. However, didn't deal with the first part of 8.
Please review.

Yogesh Pawar’s picture

Assigned: chiranjeeb2410 » Unassigned

@chiranjeeb2410 - can you also add a interdiff, so we can check what changes you have done in current patch

vaplas’s picture

Status: Needs work » Needs review
FileSize
189.78 KB
6.39 KB

@klausi!!! #89 Absolutely EPIC clenup! Now all setExpectedException inserting before last operation! Looks like bye-bye my algos) Now I understand why you proposed to do it outside of this issue ;)

vaplas’s picture

@chiranjeeb2410, thank you for help! @Yogesh Pawar +1, interdiff would be very helpfull.

Yogesh Pawar’s picture

Thanks @vaplas - for the updated patch.

klausi’s picture

chiranjeeb2410’s picture

Status: Needs work » Needs review
FileSize
157.64 KB

@vaplas, thanks for the updated patch. Applies cleanly and changes are adequate. Also @klausi, +1 for #89.
Uploading previous interdiff as part of procedure.

chiranjeeb2410’s picture

Status: Needs review » Needs work
klausi’s picture

Issue tags: +Needs reroll

We still need a reroll of #92.

vaplas’s picture

Status: Needs review » Needs work

The last submitted patch, 99: 2822837-99.patch, failed testing.

vaplas’s picture

Status: Needs work » Needs review
FileSize
189.78 KB
632 bytes

oops, sorry.

chiranjeeb2410’s picture

Status: Needs review » Reviewed & tested by the community

@vaplas,

Thanks for the reroll. Patch applies cleanly. Changes are met with.
Updating to RTBC.

dawehner’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Postponed

Let's get #2859286: Forbid @expectedException @expectedExceptionMessage done first and then core upgraded to use a version of coder with this in. Otherwise we are certain to regress.

klausi’s picture

That is done in Coder and I made a new release. The next step is to get #2861793: Upgrade Coder to 8.2.12 in.

klausi’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Postponed » Needs work

That got committed, yay!

We need a reroll here + an entry for the new sniff in phpcs.xml.dist.

vaplas’s picture

Status: Needs work » Needs review
FileSize
188.35 KB
Mile23’s picture

Status: Needs review » Needs work

Nice. Still needs the changes to core/phpcs.xml.dist, however.

vaplas’s picture

opps))) Thank you, @Mile23!

vaplas’s picture

Status: Needs work » Needs review

And #2861793: Upgrade Coder to 8.2.12 now works for 8.3.x too!

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Nice! Tested manually that the new sniff triggers correctly if such an annotation is in the code base, so this is good to go.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 4656717 to 8.4.x and c302032 to 8.3.x. Thanks!

  • alexpott committed 4656717 on 8.4.x
    Issue #2822837 by vaplas, martin107, chiranjeeb2410, klausi, dawehner:...
vaplas’s picture

Thank you everyone!!!

PS. Drinking in the middle of the working day of the beginning of the week is not the best idea, but what should I do ?!) Beaver Dance!

Status: Fixed » Closed (fixed)

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