Skip to content

Add a test for command ListUsers (test emails) #1133

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

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

Nek-
Copy link
Contributor

@Nek- Nek- commented Jul 13, 2020

The command list user was not tested until now, and that's quite sad because it's one of the only parts of the code that actually sends an email. Symfony has some tools for testing emails, let's use them!

I also refactored a little bit of command testing since there was an identical piece of code between test cases. (instantiating the command)

@Nek- Nek- changed the title feat(test): add test for command list user Add a test for command ListUsers Jul 13, 2020
@Nek- Nek- changed the title Add a test for command ListUsers Add a test for command ListUsers (test emails) Jul 13, 2020
@Nek- Nek- force-pushed the feature/email-test branch from ad9ff81 to 409725d Compare July 20, 2020 10:43
@Nek-
Copy link
Contributor Author

Nek- commented Jul 20, 2020

Thank you @ker0x for feedbacks. This is fixed.

@Nek- Nek- force-pushed the feature/email-test branch from 409725d to 13611af Compare July 31, 2020 17:00
@nicolas-grekas nicolas-grekas changed the base branch from master to main November 19, 2020 12:36
@Nek- Nek- force-pushed the feature/email-test branch 2 times, most recently from fc39410 to 924445d Compare July 6, 2021 17:41
@Nek-
Copy link
Contributor Author

Nek- commented Jul 6, 2021

@javiereguiluz rebased!

@javiereguiluz
Copy link
Member

Maxime, thanks for providing this test and for your work in the review phase. Thanks to reviewers too!

@javiereguiluz javiereguiluz merged commit c016ffc into symfony:main Jul 7, 2021
protected function setUp(): void
{
if ('Windows' === \PHP_OS_FAMILY) {
$this->markTestSkipped('`stty` is required to test this command.');
Copy link
Member

Choose a reason for hiding this comment

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

Moving this to the abstract class is wrong. Not all command tests require stty. And AFAICT, testing the list:users command will not require stty as it is not an interactive command.

Copy link
Member

Choose a reason for hiding this comment

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

Mmmmm, you are right! Anybody willing to make a Pull Request to fix this? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in #1230

@Nek- Nek- deleted the feature/email-test branch July 10, 2021 19:25
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.

6 participants