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

33295: Eliminate AspectMock from FileStorageTest #842

Conversation

karyna-tsymbal-atwix
Copy link
Contributor

@karyna-tsymbal-atwix karyna-tsymbal-atwix commented Jul 1, 2021

Eliminated AspectMock usage from dev/tests/unit/Magento/FunctionalTestFramework/DataGenerator/Handlers/SecretStorage/FileStorageTest.php

note: in this case, main complexity is in a private functions calls rather than static

Fixed Issues (if relevant)

  1. Fixes [MFTF] Eliminate AspectMock from FileStorageTest (Complex!) magento2#33295: [MFTF] Eliminate AspectMock from FileStorageTest (Complex!)

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

@karyna-tsymbal-atwix karyna-tsymbal-atwix changed the title Eliminate AspectMock from FileStorageTest 33295: Eliminate AspectMock from FileStorageTest Jul 1, 2021
@magento-engcom-team magento-engcom-team added Partner: Atwix partners-contribution Pull Request is created by Magento Partner labels Jul 1, 2021
andrewbess
andrewbess previously approved these changes Jul 1, 2021
@andrewbess
Copy link
Contributor

Hello @karyna-tsymbal-atwix
Thank you for your PR.
It looks well to me.

@andrewbess andrewbess force-pushed the improvement/mftf-33295-eliminate-aspect-mock-from-fileStorageTest branch from efb91f5 to e8bbf77 Compare July 1, 2021 13:01
andrewbess
andrewbess previously approved these changes Jul 1, 2021
@jilu1
Copy link
Contributor

jilu1 commented Jul 1, 2021

@magento-engcom-team
Copy link

@jilu1 the pull request successfully imported.

@bohdan-harniuk
Copy link
Contributor

Hello, @jilu1

This task was finished without creating new singletons, so it is not a blocker.

Thanks, Bohdan

bohdan-harniuk
bohdan-harniuk previously approved these changes Jul 22, 2021
jilu1
jilu1 previously approved these changes Jul 22, 2021
Copy link
Contributor

@jilu1 jilu1 left a comment

Choose a reason for hiding this comment

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

@karyna-tsymbal-atwix
Thank you for your contribution!
This change looks good to me.

* @throws TestFrameworkException
*/
public function __construct()
private function initialize(): void
Copy link
Contributor

Choose a reason for hiding this comment

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

@karyna-tsymbal-atwix
Can we try not to change the constructor to initialize()? Do you have some hard dependencies that are difficult to mock?

If it's difficult to find other solutions, please make sure initialization is done properly here:

private function initializeFileStorage()
    {
        // Initialize file storage
        try {
            $this->credStorage[self::ARRAY_KEY_FOR_FILE]  = new FileStorage();
        } catch (TestFrameworkException $e) {
            // Print error message in console
            print_r($e->getMessage());
            // Save to exception context for Allure report
            $this->setExceptionContexts(self::ARRAY_KEY_FOR_FILE, $e->getMessage());
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jilu1 , the problem here is that we need real private methods from the constructor to be executed (including parent constructor), but with our test credentials here https://github.com/magento/magento2-functional-testing-framework/pull/842/files#diff-ae8cfe83e67da69395e70368c54[…]25516f88e60225a94aa557060dda04L31

I didn't find a proper way to avoid calling a real constructor and execute parent constructor&private methods from constructor, at the same time.
Regarding your question about initializeFileStorage() method - I was checking it's usages, and I think it should be ok.
In the FileStorage class, an entry point is getEncryptedValue() method. I think it's the only case when we need those methods from constructor (which I moved to initialize()) to be executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@karyna-tsymbal-atwix
I am ok with using initialize() if there is no easy to mock private method in this case.

Practically getEncryptedValue() is the only entry point with the assumption that other public methods inherited from base class should be called after, but logically initialize() should be called right after FileStorage instantiation in initializeFileStorage() here

. I would add initialize() in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jilu1 PR is updated with your suggestion, thank you

@jilu1 jilu1 added the accept label Jul 28, 2021
@magento-devops-reposync-svc magento-devops-reposync-svc merged commit abaa8b4 into magento:develop Jul 28, 2021
@karyna-tsymbal-atwix karyna-tsymbal-atwix deleted the improvement/mftf-33295-eliminate-aspect-mock-from-fileStorageTest branch September 30, 2021 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accept in progress Partner: Atwix partners-contribution Pull Request is created by Magento Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MFTF] Eliminate AspectMock from FileStorageTest (Complex!)
6 participants