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

MAGETWO-46837: Implementing extension to wait for readiness metrics t… #161

Merged

Conversation

pdohogne-magento
Copy link
Contributor

Code review for MAGETWO-46837 Readiness Check extension

@magento-cicd2
Copy link

magento-cicd2 commented Jul 3, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Jul 3, 2018

Coverage Status

Coverage decreased (-0.03%) to 56.173% when pulling b462aa8 on magento-borg:MAGETWO-46837-readiness-flag-develop into 45d802b on magento:develop.

*
* @var TestInterface
*/
private $test;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of storing the test param, what do you think about just keeping it as a function variable for each of your event functions? I see the only place you use it where you don't have access to an event is in the logDebug function, but you could pass it in as an arg instead of relying on the class to store the test variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That depends on how Codeception handles events. I use the test object from the beforeTest event to track metric failure data ( $test->getMetadata()->setCurrent() ), so if the test object attached to the step events gets recreated or the metadata isn't otherwise persisted between events then that tracking functionality is lost.

I could move that tracking out of the test object and into an extension-specific construct directly instead, though I'd still be passing that around between metrics. Any objections?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's a good point. I guess I'd only be concerned about tethering ourselves to codeception's test object for your metadata, but I don't feel that strongly about it atm. @okolesnyk or @magterskine any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@pdohogne-magento @imeron2433 @magterskine
I think in our case when we need to release this next week we can leave it as it is for now. And probably do the refactoring when the story about skip metrics attribute for actions will be in progress.

@pdohogne-magento pdohogne-magento force-pushed the MAGETWO-46837-readiness-flag-develop branch from 9fe3f3d to 0291d36 Compare July 12, 2018 22:37
@pdohogne-magento pdohogne-magento force-pushed the MAGETWO-46837-readiness-flag-develop branch from 0291d36 to ff467df Compare July 13, 2018 03:20
@KevinBKozan KevinBKozan merged commit 5733a1b into magento:develop Jul 16, 2018
magento-devops-reposync-svc pushed a commit that referenced this pull request Feb 17, 2022
MQE-3228: Update MFTF to not pass NULL into non-nullable arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants