Skip to content

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 :)

@AlexSkrypnyk AlexSkrypnyk merged commit b2de4f0 into jhedstrom:main Jan 21, 2026
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.

3 participants