There is code in that test that is not running, it is hasn't been running since 2013. We should either fix or remove that code.

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

Status: Active » Needs review
StatusFileSize
new2.04 KB

This is one options, just removing the code.

borisson_’s picture

This was discovered in #2937844: Fix 'Squiz.PHP.NonExecutableCode' coding standard. Should this be in the phpunit component?

idebr’s picture

Status: Needs review » Reviewed & tested by the community

The debug function was removed in #2212081: Remove views optional handler handling, so it is safe to assume this code is no longer needed.

alexpott’s picture

Well it was commented out in #1822048: Introduce a generic fallback plugin mechanism.

It'd be great to get a +1 from a sub system maintainer. I think this is the right thing to do after reading #2212081: Remove views optional handler handling.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2947588.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Reviewed & tested by the community

Testbot Snafu.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Still waiting for the subsystem maintainer review.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Status: Needs review » Needs work

These tests should be in, because they test the 'Broken/Missing handler' UI text that you get when there's a problem.

So what we should do here, I think, is try removing the return, see what happens. I suspect the text the test is checking for will need tweaking, as BrokenHandlerTrait doesn't quite have the same text.

andypost’s picture

Status: Needs work » Needs review
Related issues: +#2937844: Fix 'Squiz.PHP.NonExecutableCode' coding standard
StatusFileSize
new5.65 KB

New patch with different approach - refactoring of test code

- added testing of messages from \Drupal\views\Plugin\views\BrokenHandlerTrait cos there's no actual testing of the trait and #2212081-24: Remove views optional handler handling tells about it
- extended tests for broken handler for both cases - when table is broken and when field is broken

Not sure it needs subsytem maintainer review

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The does not apply for 9.1, therefore a reroll is needed.

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new5.66 KB

Rerolled patch for 9.1.x, please review.

Status: Needs review » Needs work

The last submitted patch, 17: 2947588-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ravi.shankar’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Setting it to NR and removing needs reroll tage.

daffie’s picture

Status: Needs review » Needs work

The reroll looks good, just one nitpick:

+++ b/core/modules/views/tests/src/Kernel/ModuleTest.php
@@ -2,14 +2,19 @@
 
 class ModuleTest extends ViewsKernelTestBase {

The empty line before the class statement needs to go.

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new5.67 KB
new331 bytes

Updated patch as per the changes mentioned in #20, please review.

shaktik’s picture

StatusFileSize
new5.67 KB

Hi @daffie,

Just removed empty line before the class statement, Thanks for the review.

shaktik’s picture

Hi @mrinalini9,

apologize for the inconvenience patch IS not updated one day, that why I picked and updated.

mrinalini9’s picture

Hi @shaktik,
I have already rerolled patch in #17, and also today, I am able to apply the same patch after taking the latest pull from the branch. So I don't think any update was needed. Can you please add interdiff file here.
Thanks

daffie’s picture

@mrinalini9: thank you for the patch.

@shaktik: Please post an interdiff.txt with your patch.

shaktik’s picture

Hi @daffie, @mrinalini9,

below the interdiff as feedback on #20.

@@ -15,7 +15,6 @@
  *
  * @group views
  */
-
 class ModuleTest extends ViewsKernelTestBase {
 
   /**
mrinalini9’s picture

Hi @shaktik,
I have already done these changes in #22 and I think both #22 and #23 are the same patches, I didn't found any differences between them.
Thanks

lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

This looks great. No overlap with existing coverage, so refactoring this looks like the better solution then removing it.

The current scope here is just to refactor the old test, but we should probably extend the coverage for this to also include sort and argument:

  • \Drupal\views\Plugin\views\sort\Broken
  • \Drupal\views\Plugin\views\argument\Broken

But I'm fine with it if people want to move that to a follow up.

catch’s picture

Title: Remove code that doesn't run from \Drupal\Tests\views\Kernel\ModuleTest::testViewsGetHandler » Refactor \Drupal\Tests\views\Kernel\ModuleTest::testViewsGetHandler
alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0c700b50a0 to 9.1.x and bf8bbd989e to 9.0.x and ebbda37025 to 8.9.x. Thanks!

Backported to 8.9.x as a test only change.

  • alexpott committed 0c700b5 on 9.1.x
    Issue #2947588 by mrinalini9, borisson_, shaktik, andypost, daffie,...

  • alexpott committed bf8bbd9 on 9.0.x
    Issue #2947588 by mrinalini9, borisson_, shaktik, andypost, daffie,...

  • alexpott committed ebbda37 on 8.9.x
    Issue #2947588 by mrinalini9, borisson_, shaktik, andypost, daffie,...

Status: Fixed » Closed (fixed)

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