search

ubirak / rest-api-behat-extension

Stuff to easily test your rest api with Behat
MIT License
38 stars 24 forks source link

Updated status code step definition to not conflict with drupal extension. #97

Closed AlexSkrypnyk closed 3 years ago

AlexSkrypnyk commented 6 years ago

Then the response status code should be step definitions breaks tests for Drupal site using behat drupal extensions because both extensions define this step definition.

This PR resolves the issue by changing this step definition to include the word rest.

I realise that chances to have this PR merged are low, but at least this can serve as a reference for other developers.

bystro commented 6 years ago

Yeah! It's good to know. The merge would require too much refactoring of existing scenario implementations.

Let's think about an equivalent so that have them both

Then the response status code should be 200
Then the rest response status code should be 200
AlexSkrypnyk commented 6 years ago

@bystro The merge would require too much refactoring of existing scenario implementations. - are you referring to the changes in projects that already using this extension? If so - yeah, they would need to change, but since this plugin can be released as a separate version, they code will not be broken unless they composer update.

There are a lot of headless projects are currently developed In Drupal community. If they use behat testing, they are using drupalextension, but they need to use something for API testing. Currently, they cannot use this project because of this single issue. I'm just pointing out the impact.

BTW, behat extensions sometimes change their step definitions and it should be expected by the developers.

bystro commented 6 years ago

I recommend to maintain both versions in the master branch.

tyx commented 6 years ago

@alexdesignworks Thanks you for reporting this issue

Please, can you add a link to the conflicted step in their code ? I did not find it

AlexSkrypnyk commented 6 years ago

@tyx the pr diff has the path to the file where the change needs to be applied

AlexSkrypnyk commented 5 years ago

@tyx apologies, I misread your message. here it is in their repo https://github.com/jhedstrom/drupalextension/blob/156fb907d73fbcc1348e848e26395659fdd6171c/src/Drupal/DrupalExtension/Context/MinkContext.php#L562

tyx commented 5 years ago

@alexdesignworks np

The step you linked is I should not get a :code HTTP response so no conflict with ours the response status code should be ?

I did not find in this MinkContext anything that could conflict.

AlexSkrypnyk commented 5 years ago

@tyx My apologies for confusing you - I looked at the wrong file.

Please see this one: https://github.com/Behat/MinkExtension/blob/master/src/Behat/MinkExtension/Context/MinkContext.php#L266

The conflict is with behat/mink-extension project. drupal-extension project extends MinkContext class and has Then the response status code should be step definition declared. So it is still a valid case of a conflicting steps. :(

I have to leave my fork available to projects until this PR is merged.

tyx commented 5 years ago

Hello @AlexSkrypnyk

Ok I get the issue !

However, this extension is clearly here to replace MinkContext, not to work with it. Same purpose but different way.

I cannot accept your PR, it's a major BC break for a minimal benefit :(

In my mind, you rather should have different behat profil/suite, where you load either DrupalContext either our context. Mixing them looks a bad idea !