Follow-up to #2536560: Runtime Assertion unit and functional testing

Credit: Fabianx

  assert(FALSE);

Fails with:

Missing argument 4 for Drupal\Component\Assertion\Handle::Drupal\Component\Assertion\{closure}()

CommentFileSizeAuthor
#9 2561207-9.diff2.4 KBAki Tendo
#3 2561207-1.patch1.46 KBstefan.r
#3 2561207-testonly.patch828 bytesstefan.r

Comments

stefan.r created an issue. See original summary.

fabianx’s picture

fabianx commented

Thanks for creating the issue!

Just saying: assert() is overall a pure joy to use.

stefan.r’s picture

stefan.r commented
StatusFileSize
new 2561207-testonly.patch828 bytes
new 2561207-1.patch1.46 KB

Both of these should fail...

The last submitted patch, 3: 2561207-testonly.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

I think the only hope for assert() is that more people are using it, so php will not treat it as second level thing any longer.

Status: Needs review » Needs work

The last submitted patch, 3: 2561207-1.patch, failed testing.

valthebald’s picture

valthebald
Location Sofia
commented 
  assert(FALSE);

is not failing with default PHP install in Debian stable (Apache 2.4.10 module, PHP 5.6.12)
What exactly is failing PHP 5 version?

fabianx’s picture

fabianx commented

5.5.9 on Ubuntu LTS

Here is the problem:

https://3v4l.org/bpunR

It is just a warning, but phpunit really does not like warnings during tests.

Aki Tendo’s picture

Aki Tendo commented
Component: base system » simpletest.module
Status: Needs work » Needs review
Issue tags: -Needs tests +Runtime assertion
StatusFileSize
new 2561207-9.diff2.4 KB

Adjusted handler to have default arguments, adjusted SimpleTestTest to assert with and without a message. Thanks Fabianx.

(If this passes, minor error in patch - I am still confusing insure/ensure in my comments. I don't know why.)

stefan.r’s picture

stefan.r commented

Ideally we'd want a test only patch that triggers the warning as well, but maybe this depends on PHP versions and PHP on testbots doesnt care...

Aki Tendo’s picture

Aki Tendo commented

No we don't. That warning is a mishandling of the error state and just confuses the issue. The handler should not ever return an argument error just because the assertion writer failed to include an assertion message.

I am testing that scenario now - assert without a message - but that shouldn't cause PHP to have a warning - it should just work. I'm also testing the prior scenario - assert with a message - as well.

stefan.r’s picture

stefan.r commented

Well as we can't replicate the failure but Fabianx did so manually (and got rid of it in manual testing through essentially the fix in this patch) I think this patch looks great then now. It merely adds default arguments to the exception class. I'd RTBC but I've already posted a patch :)

I do wonder if it's a major considering we can't reproduce. But it's a followup to a major so this should be good for the beta.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

This is simple enough to RTBC:

Fail:

https://3v4l.org/bpunR

Pass:

https://3v4l.org/MjoBX

=> Works.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/simpletest/src/Tests/SimpleTestTest.php
@@ -167,11 +167,20 @@ function stubTest() {
+        // Now test with an error message to insure it is correctly passed

s/insure/ensure, as mentioned in #9

Could be fixed on commit if needed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2561207-9.diff, failed testing.

stefan.r’s picture

stefan.r commented
Status: Needs work » Reviewed & tested by the community

Back to RTBC on the assumption this was an unrelated test fail:

file_put_contents(/tmp/test-file.jpg): failed to open stream: Permission deniedfile_put_contents('/tmp/test-file.jpg', '') Drupal\file\Tests\Migrate\EntityFileTest->setUp() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('438', 'Drupal\file\Tests\Migrate\EntityFileTest')

Aki Tendo queued 9: 2561207-9.diff for re-testing.

Aki Tendo’s picture

Aki Tendo commented

Retesting just to be sure.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 70880e9 on 8.0.x 
    Issue #2561207 by stefan.r, Aki Tendo: assert() without a message breaks...
Aki Tendo’s picture

Aki Tendo commented
Issue tags: +Quickfix

"Insure" should be "ensure" in the comments. Sorry about that - I thought it could be dealt with at commit time. Should I make a new patch for the typo?

catch’s picture

catch
he/him
Primary language English
commented

I missed that.

New minor followup is great.

Status: Fixed » Closed (fixed)

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