Problem/Motivation

Currently Views intent (derivatives) don't have test coverage, so we need to add some tests to sleep better during the night.

Remaining tasks

- Wait for #2905388: The "view" entity type does not exist to be in
- Write a test

User interface changes

No.

API changes

No.

Data model changes

No.

CommentFileSizeAuthor
#5 add_test_coverage_for-2911375-5.patch10.97 KBgambry

Comments

gambry created an issue. See original summary.

gambry’s picture

Issue summary: View changes
larowlan’s picture

Should be able to make this a kernel test if we add another test module that has a test view in it.

gambry’s picture

Yep, that's the plan. Also we should add a basic test handling the pager/iterator, althought I would like to improve and better document that bit.

gambry’s picture

Status: Active » Needs review
Issue tags: +Needs followup
StatusFileSize
new10.97 KB

This patch:
- Add configuration schema for views.display.chatbot_intent, required in order to run the test.
- Add a test module with a views with a chatbot_intent display.
- Add a test, asserting the views intent is loaded, processed and pager/iterator works.

Remaining tasks:
- Test doesn't cover potential filters coming from slots. This is because I noticed filters values are scanned in both context and slots even if those filters are not exposed. I'm not sure if this is a feature or a bug. Additionally value casting to FALSE - i.e. status = 0 - even if valid are skipped on ViewsIntent::processFilters(). I think the filters processor needs a bit of love in a followup.
- Assertion are inside closure. I like it, however it makes really difficult to debug in case of failure as phpunit prints out errors like "Fatal error: Uncaught Exception: Serialization of 'Closure' is not allowed in -:68". We may review those in here or a followup.

larowlan’s picture

+++ b/tests/src/Kernel/ChatbotViewsIntentTest.php
@@ -0,0 +1,149 @@
+    $class = $this;
+    $response->setIntentResponse(Argument::type('string'))->will(function ($args) use ($class) {
+      $class->assertSame($args[0], 'Alice');
+    });

I think $this inside the closure still corresponds to the test class

other than that, looks good - agree the closure is painful, but can't think of another way

gambry’s picture

@larowlan I tried with $this inside the closure, and got this error:

Prophecy\Exception\Doubler\MethodNotFoundException: Method `Double\IntentResponseInterface\P1::assertSame()` is not defined.

agree the closure is painful, but can't think of another way

The other way would be to define variables, passing to the closures as references and asserting them after the prophecies ran, i.e.

$output = '';
$response->setIntentResponse(Argument::type('string'))->will(function ($args) use (&$output) {
$output = $args[0];
});
[...]
$this->assertSame($output, 'Alice');

Code is slightly less clean, but phpunit outputs exception messages as you would expect.

gambry’s picture

Issue tags: +Vienna2017

  • gambry authored 47771c3 on 8.x-1.x
    Issue #2911375 by gambry, larowlan: Add test coverage for Views intent
    
gambry’s picture

Status: Needs review » Fixed

As we need to make some progress in here I've committed #5 taking in consideration #6.1 (using directly $this in the closure) returns an error so there were no outstanding feedback.

I still think the clueless error of asserting inside the closure is potentially dangerous. Solution in #7 works but I'm not sure how good practice is to pass "pawns" as reference to be changed within closure and asserting their values later.
We can always change it later.

Thanks!

Status: Fixed » Closed (fixed)

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