Skip to content
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

Merged

Conversation

lbajsarowicz
Copy link
Contributor

@lbajsarowicz lbajsarowicz commented Jan 31, 2020

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.

  • Reorganized tests and it's extensions to work properly.
  • Removed the cache:flush bad practice loved by Core Team <3
  • Fixed issues caused by missing indexing (adding cron:run --group=index)
  • Invalid search returns returned (duplicated products)
  • Extended Credit Card expiration period to another 4 years

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)

  1. MFTF: AdminReorderWithCatalogPriceTest is failing in CI process #26607

Manual testing scenarios (*)

  1. Change README.md for any module
  2. Submit PR to Github

Questions or comments

🤦‍♂️

Contribution checklist (*)

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

@m2-assistant
Copy link

m2-assistant bot commented Jan 31, 2020

Hi @lbajsarowicz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@magento-engcom-team magento-engcom-team added the partners-contribution Pull Request is created by Magento Partner label Jan 31, 2020
@ihor-sviziev ihor-sviziev self-assigned this Jan 31, 2020
@lbajsarowicz lbajsarowicz changed the title #26607 Fix failing CI due to missing "createSimpleProductApi" #26607 Fix failing CI due to Functional Tests Feb 1, 2020
@lbajsarowicz lbajsarowicz changed the title #26607 Fix failing CI due to Functional Tests HOTFIX: #26607 Fix failing CI due to Functional Tests Feb 1, 2020
@lbajsarowicz
Copy link
Contributor Author

@magento run all tests

1 fixed = 1 new to be fixed 🤦‍♂️
And all the issues are similar and are result of invalid test isolation

@lbajsarowicz
Copy link
Contributor Author

@magento run Functional Tests CE

Reason:
image

@Nazar65
Copy link
Member

Nazar65 commented Feb 2, 2020

@magento run Functional Tests CE

Reason:
image

Yes i had exact same issue when i was trying to fix failed b2b on my PR

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Feb 2, 2020

Working on it - the structure of the tests related to searching products are broken even if it fails

@lbajsarowicz
Copy link
Contributor Author

@magento run all tests

I've removed sample products that are not used for testing, as it breaks all Elasticsuite tests 🤦‍♂️

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Feb 2, 2020

The issue of Tests Isolation is the issue, still.
The easiest way to avoid issues with searching by short description is to remove spaces from ShortDescription as it makes search catch other products.

There should be confirmation when accidentally click "Close" button 🤦‍♂️

@magento-engcom-team
Copy link
Contributor

Hi @lenaorobei, thank you for the review.
ENGCOM-6825 has been created to process this Pull Request

@lbajsarowicz
Copy link
Contributor Author

@magento run all tests

…i-failure

# Conflicts:
#	app/code/Magento/Sales/Test/Mftf/Test/AdminReorderWithCatalogPriceRuleDiscountTest.xml
@lbajsarowicz
Copy link
Contributor Author

@magento run all tests
🙏 🙏 🙏

@m2-assistant
Copy link

m2-assistant bot commented Feb 22, 2020

Hi @lbajsarowicz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@zakdma
Copy link
Contributor

zakdma commented Feb 24, 2020

@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.
Also there is special AG for this purposes
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Indexer/Test/Mftf/ActionGroup/CliRunReindexUsingCronJobsActionGroup.xml

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Feb 24, 2020

@zakdma

  1. I am not running reindexes
  2. I am running Cron Jobs - which is exactly the same thing that happens on Production Environments

Cron Jobs from index group are responsible for updating single entities indexes based on _cl tables that are created once you configure reindex to run on schedule instead of realtime. Every single time when you create or modify products, you have to give the indexers the opportunity to update the indexes. Magento Core way is to run indexer:reindex and cache:flush which is a very expensive way to get the same work done.

I'd love to know your point of view.

@lbajsarowicz
Copy link
Contributor Author

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 cron:run twice, and it will be executed in the same minute - second one is going to be skipped.

@zakdma
Copy link
Contributor

zakdma commented Feb 24, 2020

No, you are not right. First run schedule jobs and second one run scheduled jobs. Even in the same minute

@zakdma
Copy link
Contributor

zakdma commented Feb 24, 2020

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.

@lbajsarowicz
Copy link
Contributor Author

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:

[2020-02-24 12:02:38] report.INFO: Cron Job indexer_reindex_all_invalid is run [] []
[2020-02-24 12:02:39] report.INFO: Cron Job indexer_reindex_all_invalid is successfully finished. Statistics: {"sum":0.32009887695312,"count":1,"realmem":14680064,"emalloc":16307456,"realmem_start":48758784,"emalloc_start":45042280} [] []
[2020-02-24 12:02:39] report.INFO: Cron Job indexer_update_all_views is run [] []
[2020-02-24 12:02:39] report.INFO: Cron Job indexer_update_all_views is successfully finished. Statistics: {"sum":0.40363311767578,"count":1,"realmem":10485760,"emalloc":6242624,"realmem_start":63438848,"emalloc_start":61352328} [] []
[2020-02-24 12:03:06] report.INFO: Cron Job indexer_reindex_all_invalid is run [] []
[2020-02-24 12:03:06] report.INFO: Cron Job indexer_reindex_all_invalid is successfully finished. Statistics: {"sum":0.015038967132568,"count":1,"realmem":0,"emalloc":905128,"realmem_start":46661632,"emalloc_start":44110200} [] []
[2020-02-24 12:03:06] report.INFO: Cron Job indexer_update_all_views is run [] []
[2020-02-24 12:03:06] report.INFO: Cron Job indexer_update_all_views is successfully finished. Statistics: {"sum":0.1584370136261,"count":1,"realmem":6291456,"emalloc":6716520,"realmem_start":46661632,"emalloc_start":45017920} [] []
[2020-02-24 12:04:00] report.INFO: Cron Job indexer_reindex_all_invalid is run [] []
[2020-02-24 12:04:00] report.INFO: Cron Job indexer_reindex_all_invalid is successfully finished. Statistics: {"sum":0.014179229736328,"count":1,"realmem":0,"emalloc":905128,"realmem_start":46661632,"emalloc_start":44110200} [] []
[2020-02-24 12:04:00] report.INFO: Cron Job indexer_update_all_views is run [] []
[2020-02-24 12:04:00] report.INFO: Cron Job indexer_update_all_views is successfully finished. Statistics: {"sum":0.17619705200195,"count":1,"realmem":6291456,"emalloc":6716520,"realmem_start":46661632,"emalloc_start":45017920} [] []
[2020-02-24 12:05:05] report.INFO: Cron Job indexer_reindex_all_invalid is run [] []
[2020-02-24 12:05:05] report.INFO: Cron Job indexer_reindex_all_invalid is successfully finished. Statistics: {"sum":0.012212991714478,"count":1,"realmem":0,"emalloc":905128,"realmem_start":46661632,"emalloc_start":44110200} [] []
[2020-02-24 12:05:05] report.INFO: Cron Job indexer_update_all_views is run [] []
[2020-02-24 12:05:05] report.INFO: Cron Job indexer_update_all_views is successfully finished. Statistics: {"sum":0.14868688583374,"count":1,"realmem":6291456,"emalloc":6716520,"realmem_start":46661632,"emalloc_start":45017920} [] []
[2020-02-24 12:06:04] report.INFO: Cron Job indexer_reindex_all_invalid is run [] []
[2020-02-24 12:06:04] report.INFO: Cron Job indexer_reindex_all_invalid is successfully finished. Statistics: {"sum":0.016084909439087,"count":1,"realmem":0,"emalloc":905128,"realmem_start":46661632,"emalloc_start":44110200} [] []
[2020-02-24 12:06:04] report.INFO: Cron Job indexer_update_all_views is run [] []
[2020-02-24 12:06:04] report.INFO: Cron Job indexer_update_all_views is successfully finished. Statistics: {"sum":0.17505884170532,"count":1,"realmem":6291456,"emalloc":6716520,"realmem_start":46661632,"emalloc_start":45017920} [] []

I'd love to learn something new - so let me understand your point of view.

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Feb 24, 2020

@zakdma This is actually sad, that you are throwing stones in my direction without even checking the triggers that are declared in mview.xml.

As an example

<view id="catalog_product_attribute" class="Magento\Catalog\Model\Indexer\Product\Eav" group="indexer">
<subscriptions>
<table name="catalog_product_entity_decimal" entity_column="entity_id" />
<table name="catalog_product_entity_int" entity_column="entity_id" />
<table name="catalog_product_entity_varchar" entity_column="entity_id" />
</subscriptions>
</view>

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 entity_id is being saved to catalogsearch_fulltext_cl (and 3 others).

The indexer is looking for changes in these tables to update indexes for single entities.

public function getList($fromVersionId, $toVersionId)
{
$changelogTableName = $this->resource->getTableName($this->getName());
if (!$this->connection->isTableExists($changelogTableName)) {
throw new ChangelogTableNotExistsException(new Phrase("Table %1 does not exist", [$changelogTableName]));
}
$select = $this->connection->select()->distinct(
true
)->from(
$changelogTableName,
[$this->getColumnName()]
)->where(
'version_id > ?',
(int)$fromVersionId
)->where(
'version_id <= ?',
(int)$toVersionId
);
return array_map('intval', $this->connection->fetchCol($select));
}

@zakdma
Copy link
Contributor

zakdma commented Feb 24, 2020

I think we don't understand each other.

  1. MFTF Tests run on environment with cron disabled
  2. First manual cron:run (on an clear database) will not run jobs but just schedule jobs
    Second manual call cron:run will actually run jobs even if it run in the same minute with first call
  3. --group=index will run job indexer_reindex_all_invalid that not just index changes (that created in tables) but totally reindex all broken indices (invalidated indices)
  4. MFTF Tests environment using Update on Save index strategy, not Update by schedule

@lbajsarowicz
Copy link
Contributor Author

  1. That is why we need to run them manually
  2. I've never had such issue. Previous <magentoCLI command="cron:run --group=index"...> and current <magentoCron groups="index"> works as expected. The only exception to <magentoCLI was the described case of Crons running on the same minute, especially for Functional tests running simultaneously.
  3. It is going to run 3 jobs:
<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>
  • indexer_reindex_all_invalid is going to run for invalid indices
  • indexer_update_all_views is going to run for outdated indices

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 indexer_update_all_views every single time when entities are modified and indexers are configured to work on schedule.

@zakdma
Copy link
Contributor

zakdma commented Feb 24, 2020

  1. It is not issue but normal behaviour
  2. But for MFTF tests make sense only indexer_reindex_all_invalid because MFTF tests env don't use triggers and tables with changes since it works in Update on Save mode

It's necessary to run Cron for indexer_update_all_views every single time when entities are modified and indexers are configured to work on schedule.

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.

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Feb 24, 2020

  1. Actually, whether MFTF should run On schedule or On save should be defined at the very beginning of each suite. Let's consider that both way:
    a) Reindex On save (realtime): cron:run --group=index does nothing
    b) Reindex On schedule: cron:run --group=index runs indexer_update_all_views

That is why after createData, updateData and to be nice - even after deleteData you should run index crons. That is also recommended when you modify entity from Admin Panel.

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 we have Integration Tests, actually.

@zakdma
Copy link
Contributor

zakdma commented Feb 24, 2020

That is why we have Integration Tests, actually.

Not all code covered by IT.
It is always better to have more than one approach to test.

@lbajsarowicz
Copy link
Contributor Author

There are no obstacles for you to contribute with your approach.
Ha! You are more than welcome to stabilize the Functional Tests.

@zakdma
Copy link
Contributor

zakdma commented Feb 24, 2020

Actually I do it almost every single day.

@sdzhepa sdzhepa mentioned this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants