#55 Throw Exception when cannot resolve URL attribute

[lbajsarowicz]

Description

As an answer to the issue reported by @okolesnyk I went step further with reporting the issue of missing replacements to URL:

Before my change, MFTF tried to open... {{AdminLoginPage}} without replacing the URL 🤦‍♂️

Testing

1. Modify LoginAsAdmin Action Group (app/code/Magento/Backend/Test/Mftf/ActionGroup/LoginAsAdminActionGroup.xml) replacing url="{{AdminLoginPage.url}}" with url="{{AdminLoginPage}}"
2. Run test
3. Expect exception on first step.

Fixed Issues (if relevant)

1. Show warning on generation when leave out .url attribute for amOnPage #55

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

[coveralls]

Coverage increased (+0.08%) to 50.966% when pulling aff5a79 on lbajsarowicz:bugfix/55-missing-url-attribute into 4462b2c on magento:develop.

[KevinBKozan]

@lbajsarowicz Thank you for your contribution! I've created an internal issue to track progress on revieweing this, I'll keep you updated with comments here.

[KevinBKozan]

Super small change around readability, otherwise initial review of changes look good.

Since this has gone from a log line to an exception, I need to make sure that this does not cause exceptions in the entire magento mftf test codebase before introducing it.

[KevinBKozan]

Hey @lbajsarowicz, I'm seeing errors on 2.4-develop CE using your branch.
They can be fixed just fine, and I don't think there's anything wrong with the code, but changing a warning to an exception constitutes a breaking change.

Good news is that we're planning on releasing a 3.0.0 soon-ish, so I propose we keep this branch as approved but not merged until we start working on the major version branch.

Does that sound fair?

[lbajsarowicz]

Yes, it's fine. Tag this as release line 3.0

[tomreece]

After merging this PR, we were unable to generate tests due to a {{Foo.url}} reference that was missing an argument. I tried everything I could think of to figure out how to fix the XML but could not find what the problem was.

We agreed to revert it so that we can have green builds on our 3.0.0 branches. We will look further into this asap.

TLDR: I'm reverting this.

[lbajsarowicz]

@tomreece Could you provide more details?
I have an environment with CE + EE + B2B + Inventory + PB + PBEE and... I have no such issue on my branch.

[tomreece]

@lbajsarowicz with your change in place we were receiving this error during test generation:

15:28:56 [Build Functional CE]  ! [NOTE] Can not resolve replacements: "{{url_rewrite_id}}".                   
15:28:56 [Build Functional CE]  !        Exception occurred parsing action at StepKey                          
15:28:56 [Build Functional CE]  !        "openUrlRewriteEditPageAddUrlRewrite" in Test                         
15:28:56 [Build Functional CE]  !        "AdminCreateCategoryUrlRewriteAndAddNoRedirectTest"                   
15:28:56 [Build Functional CE]  !        /var/www/html/app/code/Magento/UrlRewrite/Test/Mftf/Test/AdminCreateCa
15:28:56 [Build Functional CE]  !        tegoryUrlRewriteAndAddNoRedirectTest.xml

I tried to fix this by fixing the action groups that were not passing an argument to the page, but no matter what I tried to pass to the page url, it would not generate.

[lbajsarowicz]

@tomreece I have 2 - 3 huge architectural changes to Magento ongoing and 2 huge tasks as a job. When I finish them, I'd love to take a look into that.

[tomreece]

We also were a bit confused by another thing... It looks like you try to detect when arguments were expected but not passed in to a page url. You implemented some logic/regex to check this.

But we already detect this by setting parameterized="true" on the <page ../> object definition.
If parameterized="true" and arguments aren't passed to the page url, it will throw an error on test generation. We did not see the need for further detection of this.

I have open a PR where I try to accomplish what we mentioned in issue #55 --
My pr is here: #618

Can you help me understand what your PR does different compared to the one above? I feel that my PR accomplishes the original ask with much less changes.

[lbajsarowicz]

@tomreece I haven't changed logic, just extracted your Regexes to separate method, to improve the readability. I'm currently on a train, but later will have a chance to review the changes again and explain you why it fails.

    const ACTION_ATTRIBUTE_VARIABLE_REGEX_PATTERN = '/({{[\w]+\.[\w\[\]]+}})|({{[\w]+\.[\w]+\((?(?!}}).)+\)}})/';

was extracted into:

    const REGEX_SINGLE_GROUP = '[\w]+';
    const REGEX_WITH_INDEX = '[\w]+\.[\w\[\]]+';
    const REGEX_WITH_PARAM = '[\w]+\.[\w]+\((?(?!}}).)+\)';

[lbajsarowicz]

@tomreece Still cannot reproduce the issue

With my code merged to the latest develop branch.

[tomreece]

@lbajsarowicz I'm not sure what to suggest then.

It did not generate tests for me locally nor on our Jenkins build server.

Regardless, we already check for required parameters at this line:
https://github.com/magento/magento2-functional-testing-framework/blob/develop/src/Magento/FunctionalTestingFramework/Test/Objects/ActionObject.php#L777

We did not want another implementation of this in a different object.

[lbajsarowicz]

Let's skip that for now.
We'll investigate that later.