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

FEATURE: <magentoCron> command to execute Cron Jobs #538

Merged
merged 7 commits into from
Jan 9, 2020

Conversation

lbajsarowicz
Copy link
Contributor

@lbajsarowicz lbajsarowicz commented Jan 6, 2020

Description

Command executes Cron Jobs taking into consideration need of having interval between running same groups (60 seconds).

Untitled_ Jan 6 2020 3_56 PM - Edited

Use case:

<tests xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/testSchema.xsd">
    <test name="CronjobsIntervalTest">
        <magentoCron stepKey="runCronjobs1" groups="index stage"/>
        <magentoCron stepKey="notOverlappingCron" groups="magic"/>
        <comment userInput="Some Other Actions" stepKey="otherActions"/>
        <wait stepKey="otherActionsTime" time="10"/>
        <magentoCron stepKey="runCronjobs2"/>
        <magentoCron stepKey="runCronjobs3" groups="index stage"/>
    </test>
</tests>

Fixed Issues (if relevant)

N/A

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/verification tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
  • Changes to Framework doesn't have backward incompatible changes for tests or have related Pull Request with fixes to tests

@magento-engcom-team magento-engcom-team added Partner: Strix partners-contribution Pull Request is created by Magento Partner labels Jan 6, 2020
@coveralls
Copy link

coveralls commented Jan 6, 2020

Coverage Status

Coverage decreased (-0.03%) to 51.936% when pulling f3bb5c2 on lbajsarowicz:feature/cron-command into 231e58d on magento:develop.

@lbajsarowicz
Copy link
Contributor Author

What I want to achieve is to avoid constructs like in this file: https://github.com/magento/magento2/blob/0262f1428561f5c329a50bfbd62714fa58e175be/app/code/Magento/Catalog/Test/Mftf/Test/AdminProductCategoryIndexerInUpdateOnScheduleModeTest.xml

Also want to reduce amount of work necessary to work it out:
image

@soumyau soumyau self-requested a review January 6, 2020 22:54
Copy link
Contributor

@soumyau soumyau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbajsarowicz could you elaborate on the value of this feature and possible use cases supporting it as opposed to creating an action group to achieve this?

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Jan 7, 2020

My first approach used ActionGroup, but then @VladimirZaets asked me to cover my changes also for Magento EE / Magento B2B. I realised that you've introduced running Cron Jobs in... Action Groups:

image

According to the fact, that you cannot have nested ActionGroups, I had to find another approach that would let me execute Cron Jobs having in mind that should not be ran more often than with 1 minute interval.

IMO using such approach in Action Groups is not solution but workaround to the system limitation:

image

and it's extremely unclear about "What it actually does"

@okolesnyk Asked me "What if Magento change the way that bin/magento cron:run works? MFTF version will be coupled to Magento version.

So my answer is: Not, actually not. If Magento will change the way that cron:run works - with current approach, you'll be forced to update hundreds of Tests and Action Groups. Currently - using YAGNI approach, I would'nt bother with this case. But then - when Magento would change the behavior of cron:run I'd just gather the Magento version along with getting API Keys and I'd choose the strategy to work with <magentoCron group="x y z"> to be compatible with the version.

The value behind it is possibility to replace all the strange workarounds like before mentioned:
image
image
image

As on production environment you should never run full reindex or cache:flush. Production environment should be completely based on the fact, that Cron is running. Using all the...
image
image
image

Is irresponsible! ... and should be avoided, showing even with Functional Tests, that we should let cron work, then everything would look as expected! without cache:flush and cache:clean

@lbajsarowicz
Copy link
Contributor Author

What also comes to my mind is the fact that not always Cron Jobs end up correctly - sometimes it ends with "Exceeded memory limit" or other errors. With magentoCron we can make assertions on command output to make sure that no unnecessary output was thrown from the application.

Copy link
Contributor

@soumyau soumyau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, could you please add a step that cover magentoCron in one of the existing verification tests + documentation. Also some conflicts due to previous merge.

@lbajsarowicz
Copy link
Contributor Author

Sure. Will handle that tonight.

@lbajsarowicz
Copy link
Contributor Author

@soumyau Done.

Copy link
Contributor

@soumyau soumyau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, review passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants