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

Improve exception checking in Behat tests #27002

Merged

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Dec 15, 2021

Questions Answers
Branch? develop
Description? Most of the time when an exception is raised in Behat tests it blocks the workflow and we know the test is failing But since we sometimes need to assert that exceptions are thrown some CQRS commands are wrapped in try/catch blocks so that we can store the exception thrown and assert its content in the next step
The problem is that with this some failing commands could be ignored if we don't check the exception in the next step so we added a manual step to check that no exception was thrown But it's not convenient to add this step everytime and we could forget some

This PR adds an automatic management of this exception checking, it stores any exception that might have been thrown after each step, and in the next step it ensure that this exception has been correctly asserted (and therefore clean) if not it means the exception was not expected so we consider the test as failed With this new method no exception can be ignored now even they were wrongly caught
Type? refacto
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? ~
How to test? CI tests green
Possible impacts? Stricter and more stable tests

This change is Reviewable

@jolelievre jolelievre requested a review from a team as a code owner December 15, 2021 20:28
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix Refactoring Type: Refactoring labels Dec 15, 2021
@jolelievre jolelievre added the TE Category: Tests label Dec 15, 2021
@jolelievre jolelievre added this to the 8.0.0 milestone Dec 16, 2021
@jolelievre jolelievre added this to To be reviewed in PrestaShop 8.0.0 Dec 16, 2021
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior is correct but the logic is inserted into the AbstractDomainFeatureContext which becomes bigger and bigger. Should we isolate the logic in a dedicated class or trait?

@jolelievre
Copy link
Contributor Author

@matks I thought about it, but it means we have to always associate this other Context when we use the lastException So if we forget to set it in the suite contexts then the whole feature is ignored and we go back to the previous state where exception were ignored sometimes

At least with this if you use setLastException you have the guaranty that everything is correctly checked

We could split the abstract into several classes or use traits, but I'm not sure it brings much value It can be refactored later if needed that's not the point of this PR

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then OK for me ✅

@PierreRambaud PierreRambaud merged commit b9c17bd into PrestaShop:develop Dec 21, 2021
@PierreRambaud
Copy link
Contributor

Thank you @jolelievre

@matks matks moved this from To be reviewed to Done in PrestaShop 8.0.0 Dec 22, 2021
@prestashop-issue-bot prestashop-issue-bot bot added the Fixed Resolution: issue closed because fixed label Dec 22, 2021
@jolelievre jolelievre deleted the improve-behat-last-exception branch June 9, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Type: Bug fix develop Branch Fixed Resolution: issue closed because fixed Refactoring Type: Refactoring TE Category: Tests
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants