-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HOTFIX: #26607 Fix failing CI due to Functional Tests #26608
HOTFIX: #26607 Fix failing CI due to Functional Tests #26608
Conversation
Hi @lbajsarowicz. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
… overlapping Cron executions
app/code/Magento/Bundle/Test/Mftf/Test/StorefrontCustomerSelectAndSetBundleOptionsTest.xml
Outdated
Show resolved
Hide resolved
@magento run all tests 1 fixed = 1 new to be fixed 🤦♂️ |
@magento run Functional Tests CE |
Yes i had exact same issue when i was trying to fix failed b2b on my PR |
Working on it - the structure of the tests related to searching products are broken even if it fails |
@magento run all tests I've removed sample products that are not used for testing, as it breaks all Elasticsuite tests 🤦♂️ |
The issue of Tests Isolation is the issue, still. There should be confirmation when accidentally click "Close" button 🤦♂️ |
Hi @lenaorobei, thank you for the review. |
@magento run all tests |
…i-failure # Conflicts: # app/code/Magento/Sales/Test/Mftf/Test/AdminReorderWithCatalogPriceRuleDiscountTest.xml
…nto hotfix/26607-ci-failure
@magento run all tests |
Hi @lbajsarowicz, thank you for your contribution! |
@lbajsarowicz it is bad practice to hide issues adding reindexing everywhere even it is not necessary. You claim core team but do actually the same. |
Cron Jobs from I'd love to know your point of view. |
I don't understand @zakdma why - in your opinion - running Cron Jobs is a bad practice? The Action Group you provided is not working as expected, because if you run |
No, you are not right. First run schedule jobs and second one run scheduled jobs. Even in the same minute |
cron:run --group=index actually just do reindex of broken indices. If indices are not broken this is not necessary to run it. Indices are broken after, for instance, creating product attribute. That is expected behaviour and we need to reindex them after that. If previous test that create attribute ran before test that depends on indices it will fail but issue not failed test but test before it. |
There is a simple way to prove your ActionGroup does not work as expected: Run: watch -n5 "bin/magento cron:run --group=index -vvv" And then watch the logs after several minutes:
I'd love to learn something new - so let me understand your point of view. |
@zakdma This is actually sad, that you are throwing stones in my direction without even checking the triggers that are declared in As an example magento2/app/code/Magento/Catalog/etc/mview.xml Lines 54 to 60 in 9fe3b8a
Creates trigger on MySQL: create or replace definer = magento@`%` trigger trg_catalog_product_entity_decimal_after_delete
after delete
on catalog_product_entity_decimal
for each row
BEGIN
SET @entity_id = (SELECT `entity_id` FROM `catalog_product_entity` WHERE `row_id` = OLD.`row_id`);
INSERT IGNORE INTO `catalogsearch_fulltext_cl` (`entity_id`) values(@entity_id);
INSERT IGNORE INTO `catalog_product_attribute_cl` (`entity_id`) values(@entity_id);
INSERT IGNORE INTO `catalogrule_product_cl` (`entity_id`) values(@entity_id);
INSERT IGNORE INTO `catalog_product_price_cl` (`entity_id`) values(@entity_id);
END;
create or replace definer = magento@`%` trigger trg_catalog_product_entity_decimal_after_insert
after insert
on catalog_product_entity_decimal
for each row
BEGIN
SET @entity_id = (SELECT `entity_id` FROM `catalog_product_entity` WHERE `row_id` = NEW.`row_id`);
INSERT IGNORE INTO `catalogsearch_fulltext_cl` (`entity_id`) values(@entity_id);
INSERT IGNORE INTO `catalog_product_attribute_cl` (`entity_id`) values(@entity_id);
INSERT IGNORE INTO `catalogrule_product_cl` (`entity_id`) values(@entity_id);
INSERT IGNORE INTO `catalog_product_price_cl` (`entity_id`) values(@entity_id);
END;
create or replace definer = magento@`%` trigger trg_catalog_product_entity_decimal_after_update
after update
on catalog_product_entity_decimal
for each row
BEGIN
SET @entity_id = (SELECT `entity_id` FROM `catalog_product_entity` WHERE `row_id` = NEW.`row_id`);
IF (NOT(NEW.`value_id` <=> OLD.`value_id`) OR NOT(NEW.`attribute_id` <=> OLD.`attribute_id`) OR NOT(NEW.`store_id` <=> OLD.`store_id`) OR NOT(NEW.`value` <=> OLD.`value`) OR NOT(NEW.`row_id` <=> OLD.`row_id`)) THEN INSERT IGNORE INTO `catalogsearch_fulltext_cl` (`entity_id`) values(@entity_id); END IF;
IF (NOT(NEW.`value_id` <=> OLD.`value_id`) OR NOT(NEW.`attribute_id` <=> OLD.`attribute_id`) OR NOT(NEW.`store_id` <=> OLD.`store_id`) OR NOT(NEW.`value` <=> OLD.`value`) OR NOT(NEW.`row_id` <=> OLD.`row_id`)) THEN INSERT IGNORE INTO `catalog_product_attribute_cl` (`entity_id`) values(@entity_id); END IF;
IF (NOT(NEW.`value_id` <=> OLD.`value_id`) OR NOT(NEW.`attribute_id` <=> OLD.`attribute_id`) OR NOT(NEW.`store_id` <=> OLD.`store_id`) OR NOT(NEW.`value` <=> OLD.`value`) OR NOT(NEW.`row_id` <=> OLD.`row_id`)) THEN INSERT IGNORE INTO `catalogrule_product_cl` (`entity_id`) values(@entity_id); END IF;
IF (NOT(NEW.`value_id` <=> OLD.`value_id`) OR NOT(NEW.`attribute_id` <=> OLD.`attribute_id`) OR NOT(NEW.`store_id` <=> OLD.`store_id`) OR NOT(NEW.`value` <=> OLD.`value`) OR NOT(NEW.`row_id` <=> OLD.`row_id`)) THEN INSERT IGNORE INTO `catalog_product_price_cl` (`entity_id`) values(@entity_id); END IF;
END; As a result, the The indexer is looking for changes in these tables to update indexes for single entities. magento2/lib/internal/Magento/Framework/Mview/View/Changelog.php Lines 145 to 166 in 31aebbf
|
I think we don't understand each other.
|
<group id="index">
<job name="indexer_reindex_all_invalid" instance="Magento\Indexer\Cron\ReindexAllInvalid" method="execute">
<schedule>* * * * *</schedule>
</job>
<job name="indexer_update_all_views" instance="Magento\Indexer\Cron\UpdateMview" method="execute">
<schedule>* * * * *</schedule>
</job>
<job name="indexer_clean_all_changelogs" instance="Magento\Indexer\Cron\ClearChangelog" method="execute">
<schedule>0 * * * *</schedule>
</job>
</group>
Still, I have no idea why are you throwing stones in my direction saying that I hide the issues. As you see above: It's necessary to run Cron for |
Again, MFTF test env works in Update on Save mode for now. About hiding. It better to know what actually make index invalid. Maybe it is a bug that will be hidden if you reindex invalidated indices every time you change something. For Ex. we found bug with invalidated indices during fixing such "flaky" MFTF test. We could just add reindex and go ahead but we make deep investigation and finally found it. Now we have tons of tests "fixed" with reindex and hidden real cause of unexpected behaviour. That is sad. |
That is why after
That is why we have Integration Tests, actually. |
Not all code covered by IT. |
There are no obstacles for you to contribute with your approach. |
Actually I do it almost every single day. |
Description (*)
During fix of #26607 with #26608 - I've noticed that the test is extended and actually - the Admin session is killed, completely ignoring the fact, that further actions that are performed in original tests still need the session.
cache:flush
bad practice loved by Core Team <3cron:run --group=index
)All above mentioned also for B2B and EE version.
Related Pull Requests
https://github.com/magento/partners-magento2ee/pull/135
https://github.com/magento/partners-magento2b2b/pull/45
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
🤦♂️
Contribution checklist (*)