feat: throw if a step provides an unexpected PyString or Table argument #1614
Conversation
acoulton
left a comment
There was a problem hiding this comment.
@jdeniau this looks great, thanks, just a couple of small comments on the implementation.
One thing this highlights though is that you won't now be able to define a step to throw a PendingException with no arguments if the feature file includes a TableNode / PyString.
For example like this in the test that's failing:
Behat/features/pretty_format.feature
Lines 417 to 420 in edb265a
We can obviously fix that test (I see why it was trying to keep the context file small, but I think it'd be clearer to make the step definitions explicit anyway).
And I think it's also OK to enforce this on end users:
- if they're using our snippets or an IDE to generate pending steps from an existing feature then the TableNode / PyString argument would usually be included in the snippet anyway so it won't affect them.
- if they've added function themselves without arguments, it will now generate an error instead of being marked as pending - but it's unlikely they've committed / pushed that to a main branch so at worst they'll need to update some existing work-in-progress.
That seems like a reasonable trade-off for making the behaviour stricter and reducing the risk of false positives in future.
@carlos-granados @stof what do you think?
Agree that this sounds reasonable and I think this is OK to go forward as soon as @jdeniau makes these small adjustments |
|
I also agree it is a reasonable tradeoff |
|
@jdeniau I think this one is actually very close to being finished, do you have time to resolve the small naming / wording changes? If not I can pick this up. Thanks :) |
|
You are right, I will finish this in the next days 👍 |
ff273e7 to
3de061c
Compare
…ode, but this argument is not expected in the context
3de061c to
8712b75
Compare
|
@acoulton I made the changes, and fix the tests. |
carlos-granados
left a comment
There was a problem hiding this comment.
Thanks @jdeniau great job
|
Hi, For the record, I added this change in our codebase and found 10 issues reported by this change on 440 tests files ! 😲 |
|
@carlos-granados The message does not include any contextual information. It took some time to debug in which file the problem resides. Thank you 
|
|
@AlexSkrypnyk apologies for this - I'd only looked at the feature file assertions in this branch (rather than the actual output) and I'd assumed we'd be showing that error message in the context of the feature & step (e.g. similar to a failed / pending step). I see now that is incorrect and at this stage of execution there is indeed no context. I've got a fix for that in #1668 and we'll try to be more careful in future. |
Fixes #1609 according to @acoulton comment