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

Add Behat\Gherkin\Node\TableNode namespace for instanceof TableNode #605

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

xurizaemon
Copy link
Contributor

@xurizaemon xurizaemon commented Mar 30, 2022

At line 67, we currently check instanceof Tablenode, but since the class isn't introduced with use we check for instanceof Drupal\DrupalExtension\Context\TableNode which doesn't exist.

Steps to reproduce

This should create a user with a random name, whose email is the same random value @example.org.

Given users:
  | name | mail |
  | <?random> | <?random>@example.org |

Currently it creates a user with literal username <?random> and email <?random>@example.org.

Proposed solution

  1. Add the use statement so we detect and populate random variables from TableNode data
  2. Transform TableNode input data so that the variables are used

@RoSk0
Copy link

RoSk0 commented Nov 28, 2022

I can confirm that proposed change is fixing random token handling in the tables.

CI fails with code style checks on unrelated to this PR files:

FILE: ...s/behat_test/src/Plugin/Field/FieldWidget/AddressFieldWidget.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
41 | ERROR | [x] Use null coalesce operator instead of ternary
    |       |     operator.
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...s/behat_test/src/Plugin/Field/FieldWidget/AddressFieldWidget.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
41 | ERROR | [x] Use null coalesce operator instead of ternary
    |       |     operator.
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

@xurizaemon , can you please rebase your branch and push again. Maybe it will pass this time?

@RoSk0
Copy link

RoSk0 commented Nov 28, 2022

It looks like a tested it poorly at first .

While adding namespace is definitely required, it doesn't fix the problem. Random token does get detected with namespace in place in beforeScenarioSetVariables(), but transformVariables() does not get called for the steps with tables. So, the page created in #633 test actually has <?random_page> title.

@xurizaemon xurizaemon force-pushed the patch-1 branch 2 times, most recently from 2dbb642 to 2a4aca1 Compare November 28, 2022 18:04
@xurizaemon
Copy link
Contributor Author

xurizaemon commented Nov 28, 2022

Updated, but I think your second comment is now saying this fix doesn't work?

The branch name indicates that this was a PR submitted via web UI so it's likely I was extracting a fix from a local codebase where I didn't have a development copy of drupalextension.

Pulled in test from your repo also, thanks.

RoSk0 and others added 3 commits November 29, 2022 07:09
At line 67, we currently check `instanceof Tablenode`, but since the class isn't introduced with `use` we check for `instanceof Drupal\DrupalExtension\Context` which doesn't exist.

This fixes that (but still doesn't quite fix transforming random tokens in TableNode values).
@xurizaemon
Copy link
Contributor Author

xurizaemon commented Nov 28, 2022

tests without the proposed fix @ https://github.com/jhedstrom/drupalextension/actions/runs/3567460065

and with proposed fix @ https://github.com/jhedstrom/drupalextension/actions/runs/3567485140

🙏🏼

nope ... need to get tests running locally I guess :)

@xurizaemon
Copy link
Contributor Author

xurizaemon commented Nov 30, 2022

Tests running locally now, and looks like this is what I was missing: table: Transforms!

Tomorrow :)

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.

2 participants