Add static test to detect blocks without name attribute #11092

ihor-sviziev · 2017-09-27T13:49:34Z

Description

This change covers an issue when block doesn't contain name attribute in layout file.
As was discussed in #10354 (comment) - need to covert missing block names issue with static test.

Fixed Issues (if relevant)

N/A

Related issues

Add missing block name to allow block customisation #10352: Add missing block name to allow block customisation
Add missing block name to allow block customisation #10354: Add missing block name to allow block customisation
#10824 add name for order items grid default renderer block #10936: Cannot add new columns to item grid in admin sales_order_view layout #10824 add name for order items grid default renderer block

Manual testing scenarios

N/A

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All automated tests passed successfully (all builds on Travis CI are green)
All new or changed code is covered with unit/integration tests (if applicable)

For now I didn't fixed all places, but we can see list of them in failed test. Will create backport PRs to 2.2, 2.1 if it's needed

ihor-sviziev · 2017-09-28T09:22:26Z

@magento-team This task wasn't added to list of static tests. How I can add it to travis?
https://travis-ci.org/magento/magento2/jobs/280767003

vkublytskyi · 2017-09-28T13:15:41Z

@ihor-sviziev Integrity tests are not executed on Travis environment:

# .travis.yml
test $TEST_SUITE = "static" && TEST_FILTER='--filter "Magento\\Test\\Php\\LiveCodeTest"' || true
if [ $TEST_SUITE != "functional" ] && [ $TEST_SUITE != "js" ]; then phpunit -c dev/tests/$TEST_SUITE $TEST_FILTER; fi

Integrity tests are executed on the internal Bamboo environment. To run test locally you may use

vendor/bin/phpunit -c dev/tests/static/phpunit.xml.dist dev/tests/static/testsuite/Magento/Test/Integrity/Layout/BlockNamesTest.php

vkublytskyi · 2017-09-28T13:38:05Z

@ihor-sviziev Are you interested in fixing new test failures?

orlangur · 2017-09-28T13:54:39Z

test $TEST_SUITE = "static" && TEST_FILTER='--filter "Magento\Test\Php\LiveCodeTest"' || true

Well, last time I runned full static tests suite on Travis it finished quite quick. I propose to expand it by removing filter in scope of this PR.

ihor-sviziev · 2017-09-29T05:54:51Z

@vkublytskyi @orlangur will add running all static tests & fix new failures

ihor-sviziev · 2017-09-29T08:57:22Z

@vkublytskyi @orlangur I updated this PR, please review it again.
I have one more question: is there any benefits to use \Magento\Framework\App\Utility\AggregateInvoker instead of data Providers?

PS: static tests are passed!

vkublytskyi · 2017-09-29T09:20:43Z

@ihor-sviziev I don't know the exact reason why AggregateInvoker is used instead of data providers but it has next differences:

PHPUnit executes data providers at test suite instantiation time (before all tests and even before applying --filter option)
Each data set from data provider counted by PHPUnit as an independent test

ihor-sviziev · 2017-09-29T13:59:54Z

@vkublytskyi let's keep AggregateInvoker for now. I just asked, maybe it was incorrect to use it.
Right now one integration test failed, but I think it's not related to my changes. Please review it

orlangur · 2017-09-29T20:12:11Z

Commits 38

Could you please logically group changes into few commits and force push to the same branch?

is there any benefits to use \Magento\Framework\App\Utility\AggregateInvoker instead of data Providers?

It was introduced by me so I know the exact answer. Data providers created too much overhead, in some tests there were hundreds to thousands cases passed, for instance each layout XML in system. The biggest problem was Bamboo artifacts sizing as it tracks statistics for each test case separately.

As there are no downsides, later it was used for a smaller data providers of only 10-20 cases as well.

ihor-sviziev · 2017-09-30T05:07:07Z

@orlangur grouped commits by module names. If it will be just two commits - it will be harder to review it . Isn't it?

orlangur · 2017-09-30T17:45:14Z

Not harder, I believe all added block names should be better in single commit (as those changes are similar, it's easy to review). One commit with enabling all static tests on Travis, one commit with test for block names, maybe there were some other changes also - like fixing some static tests besides introduced one.

Run all static tests on Travis

Fix static test failures

ihor-sviziev · 2017-10-02T05:27:41Z

@orlangur done. Please review

orlangur · 2017-10-02T16:21:41Z

app/code/Magento/Tax/view/adminhtml/layout/tax_rule_edit.xml

@@ -12,7 +12,7 @@
    </head>
    <body>
        <referenceContainer name="content">
-            <block class="Magento\Tax\Block\Adminhtml\Rule\Edit"/>
+            <block class="Magento\Tax\Block\Adminhtml\Rule\Edit" name="content.tax_rule_edit"/>
            <block class="Magento\Tax\Block\Adminhtml\Rule\Edit\Form" name="tax-rule-edit" template="Magento_Tax::rule/edit.phtml"/>


A bit confusing but changing existing name would be a BC break, right?

Not sure. As for me - this is not BC change because you hadn’t ability to customize these parts, now you will have. I don’t know any case what these changes could brake existing website

Note the line I commented on - after change names are
name="content.tax_rule_edit" and name="tax-rule-edit"

normally they should have been
name="tax-rule-edit" and name="tax-rule-edit-form"

@orlangur, @ihor-sviziev Changing a name of a block is A BiC so let's keep tax-rule-edit even if it not consistent with other names.

orlangur · 2017-10-02T16:23:58Z

dev/tests/static/testsuite/Magento/Test/Integrity/Layout/BlockNamesTest.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * Test block names exists


Copyright header should be static across all files and such comment should be placed in class PHPDoc.

Feel free to not change this now as there are a lot of such occurrences in code, it needs to be enforced and fixed once for all codebase.

Could you fix them during merging this PR?

I don't think it's worth neither mine nor your additional commit, just keep in mind for future contributions and I hope to contribute such PHPCS sniff to make all copyright headers and class comments consistent.

orlangur · 2017-10-02T16:27:53Z

@ihor-sviziev great job 👍

Did you perform e57ef72 with bare hands or there was some kind of automation, like XSLT transformation?

All internal builds passed except missing block names in EE modules. I expect @vkublytskyi will handle this.

ihor-sviziev · 2017-10-02T16:52:42Z

@orlangur I’ve done it manually. Hope it will be merged soon.

ihor-sviziev · 2017-10-04T05:26:32Z

@vkublytskyi @orlangur are these changes will be accepted? Can I prepare backport PRs for 2.2 and 2.1?

vkublytskyi · 2017-10-04T09:37:01Z

@ihor-sviziev, Yes, this PR will be accepted. It may take more time than usually as we need to fix new test failures on Magento Commerce and B2B extensions.

ihor-sviziev · 2017-10-08T07:39:40Z

@vkublytskyi could you also process #11235 together, it will be more effective?

…#11092

…#11092 - fixed test failures

ihor-sviziev force-pushed the patch-9 branch from aad01fe to 40de9fc Compare September 28, 2017 08:11

vkublytskyi self-assigned this Sep 28, 2017

vkublytskyi added this to the September 2017 milestone Sep 28, 2017

vkublytskyi added develop improvement labels Sep 28, 2017

vkublytskyi added the Progress: needs update label Sep 28, 2017

vkublytskyi removed the Progress: needs update label Sep 29, 2017

magento-team added the Progress: needs update label Sep 29, 2017

ihor-sviziev added 3 commits October 2, 2017 08:26

Add static test to detect blocks without name attribute

117287f

Run all static tests on Travis

Add static test to detect blocks without name attribute

b0f5a66

Add static test to detect blocks without name attribute

e57ef72

Fix static test failures

ihor-sviziev force-pushed the patch-9 branch from dae288c to e57ef72 Compare October 2, 2017 05:27

orlangur removed the Progress: needs update label Oct 2, 2017

orlangur self-assigned this Oct 2, 2017

orlangur reviewed Oct 2, 2017

View reviewed changes

orlangur approved these changes Oct 2, 2017

View reviewed changes

vkublytskyi approved these changes Oct 3, 2017

View reviewed changes

ihor-sviziev mentioned this pull request Oct 5, 2017

[2.2-develop] Add static test to detect blocks without name attribute #11235

Merged

4 tasks

okorshenko modified the milestones: September 2017, October 2017 Oct 5, 2017

magento-team added Progress: needs update Progress: accept and removed Progress: needs update labels Oct 6, 2017

magento-team merged commit e57ef72 into magento:develop Oct 11, 2017

magento-team pushed a commit that referenced this pull request Oct 11, 2017

MAGETWO-80287: Add static test to detect blocks without name attribute …

b6eba6a

…#11092

ihor-sviziev deleted the patch-9 branch February 2, 2018 12:18

magento-engcom-team pushed a commit that referenced this pull request Mar 16, 2018

MAGETWO-80287: Add static test to detect blocks without name attribute …

f6ddcc5

…#11092 - fixed test failures

Add static test to detect blocks without name attribute #11092

Add static test to detect blocks without name attribute #11092

Uh oh!

Conversation

ihor-sviziev commented Sep 27, 2017 • edited by vkublytskyi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues (if relevant)

Related issues

Manual testing scenarios

Contribution checklist

Uh oh!

ihor-sviziev commented Sep 28, 2017

Uh oh!

vkublytskyi commented Sep 28, 2017

Uh oh!

vkublytskyi commented Sep 28, 2017

Uh oh!

orlangur commented Sep 28, 2017

Uh oh!

ihor-sviziev commented Sep 29, 2017

Uh oh!

ihor-sviziev commented Sep 29, 2017

Uh oh!

vkublytskyi commented Sep 29, 2017

Uh oh!

ihor-sviziev commented Sep 29, 2017

Uh oh!

orlangur commented Sep 29, 2017

Uh oh!

ihor-sviziev commented Sep 30, 2017

Uh oh!

orlangur commented Sep 30, 2017

Uh oh!

ihor-sviziev commented Oct 2, 2017

Uh oh!

orlangur Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

orlangur Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

vkublytskyi Oct 3, 2017

Choose a reason for hiding this comment

Uh oh!

orlangur Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

orlangur Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

orlangur commented Oct 2, 2017

Uh oh!

ihor-sviziev commented Oct 2, 2017

Uh oh!

ihor-sviziev commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkublytskyi commented Oct 4, 2017 • edited by orlangur Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ihor-sviziev commented Oct 8, 2017

Uh oh!

Uh oh!

ihor-sviziev commented Sep 27, 2017 •

edited by vkublytskyi

Loading

ihor-sviziev commented Oct 4, 2017 •

edited

Loading

vkublytskyi commented Oct 4, 2017 •

edited by orlangur

Loading