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

Support new event assertions #25

Merged

Conversation

xEdelweiss
Copy link
Contributor

Hi! This adds tests for:

  • PR 168 (merged) - seeEventListenerIsCalled/dontSeeEventListenerIsCalled
  • PR 173 (new) - seeEvent/dontSeeEvent
    And introduces a new orphan event (UserRegisteredEvent).

In case they will be needed to verify changes.
I hope I guessed the target branch correctly😀

@xEdelweiss xEdelweiss marked this pull request as draft December 11, 2023 22:54
@TavoNiievez TavoNiievez changed the base branch from 5.4_codecept5 to main December 16, 2023 20:59
@TavoNiievez TavoNiievez changed the base branch from main to 6.2 December 16, 2023 21:00
@TavoNiievez TavoNiievez changed the base branch from 6.2 to 5.4_codecept5 December 16, 2023 21:00
@TavoNiievez
Copy link
Member

TavoNiievez commented Dec 16, 2023

Hi @xEdelweiss,

The sensio/framework-extra-bundle dependency was removed in the other branches of this repository for two reasons.
First, it was causing tests to fail on older versions of Symfony.
Second, this project was archived in February this year, so it is no longer reliable for us to base our tests on the classes of this dependency.
In other words, avoid creating tests based on the 'Sensio\Bundle\FrameworkExtraBundle\EventListener\SecurityListener' class, if this dependency is still present it is an error of omission of mine with this specific branch.

Do you think you can fix the tests without taking into account this class for the assertions?

One more thing, I have marked as skipped the seeEvent and dontSeeEvent tests, whose PR has not yet been merged. It is imperative that the functionality of the previously merged PR be validated through this PR first before proceeding.

@xEdelweiss
Copy link
Contributor Author

Hi @TavoNiievez!
Thank you for the review.

In other words, avoid creating tests based on the 'Sensio\Bundle\FrameworkExtraBundle\EventListener\SecurityListener' class, if this dependency is still present it is an error of omission of mine with this specific branch.

Do you think you can fix the tests without taking into account this class for the assertions?

Yes, I think I can. Should I try to replace all SecurityListener uses from EventsCest? Or will it be too much for this PR?

One more thing, I have marked as skipped the seeEvent and dontSeeEvent tests, whose PR has not yet been merged. It is imperative that the functionality of the Codeception/module-symfony#169 be validated through this PR first before proceeding.

Great, thanks. I wasn't sure how to handle this.

@TavoNiievez
Copy link
Member

@xEdelweiss,

Yes, I think I can. Should I try to replace all SecurityListener uses from EventsCest?

sure! please go ahead.

@TavoNiievez TavoNiievez marked this pull request as ready for review December 18, 2023 16:10
@TavoNiievez TavoNiievez merged commit a0e7bf6 into Codeception:5.4_codecept5 Dec 18, 2023
4 checks passed
@TavoNiievez
Copy link
Member

@xEdelweiss

the code is excellent,
thank you very much for contributing.

@xEdelweiss
Copy link
Contributor Author

@TavoNiievez, that's awesome to hear. Thanks for your support!

TavoNiievez pushed a commit that referenced this pull request Jan 2, 2024
TavoNiievez pushed a commit that referenced this pull request Jan 2, 2024
TavoNiievez pushed a commit that referenced this pull request Jan 2, 2024
TavoNiievez pushed a commit that referenced this pull request Jan 2, 2024
TavoNiievez pushed a commit that referenced this pull request Jan 2, 2024
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