Skip to content

Add TesterTrait::assertCommandIsSuccessful() helper #41851

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 3, 2021
Merged

Add TesterTrait::assertCommandIsSuccessful() helper #41851

merged 1 commit into from
Jul 3, 2021

Conversation

yoannrenard
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR introduces a new helper TesterTrait::assertCommandIsSuccessful() that aims to help testing the result of a command.
This is inspired by BrowserKitAssertionsTrait::assertResponseIsSuccessful

@yoannrenard yoannrenard force-pushed the add_commandIsSuccesful_helper branch from 19059dc to 01db8f0 Compare June 24, 2021 21:28
@carsonbot
Copy link

Hey!

I think @Jean85 has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@yoannrenard
Copy link
Contributor Author

It looks like tests on Travis with PHP 8.0 fail as they are run against symfony/console tags v5.2.5/6.0.x-dev which both don't contain this new method added by this PR.
Shall I move those changes into another PR in order to make them pass?

@xabbuh
Copy link
Member

xabbuh commented Jun 26, 2021

The deps=high build is allowed to fail in such a case. It will fix itself automatically when branches are merged up. For deps=low you need to modify the composer.json file of the component in question and modify version constraints in a way that no pre 5.4 release of the Console component will be installed.

@yoannrenard yoannrenard force-pushed the add_commandIsSuccesful_helper branch from 0d18b9c to db39567 Compare June 26, 2021 10:42
@yoannrenard
Copy link
Contributor Author

Thanks for your help @xabbuh
I wonder, though, if this doesn't break the BC promise...

chalasr
chalasr requested changes Jun 27, 2021
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I don't see any backward incompatible change here.
LGTM but the CHANGELOG file needs to be updated

@yoannrenard
Copy link
Contributor Author

Updated @chalasr

@yoannrenard yoannrenard requested a review from chalasr June 27, 2021 15:46
nicolas-grekas
nicolas-grekas reviewed Jul 2, 2021
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

rebase needed

src/Symfony/Component/Console/CHANGELOG.md Outdated
5.4
---

* Add `TesterTrait::assertCommandIsSuccessful()` to test command
Copy link
Member

Choose a reason for hiding this comment

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

missing space at start of line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

src/Symfony/Component/Form/composer.json
@@ -33,7 +33,7 @@
"symfony/dependency-injection": "^4.4|^5.0|^6.0",
"symfony/expression-language": "^4.4|^5.0|^6.0",
"symfony/config": "^4.4|^5.0|^6.0",
"symfony/console": "^4.4|^5.0|^6.0",
"symfony/console": "^5.4|^6.0",
Copy link
Member

Choose a reason for hiding this comment

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

the conflict line should also be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/Symfony/Bundle/FrameworkBundle/composer.json
@@ -38,7 +38,7 @@
"doctrine/persistence": "^1.3|^2.0",
"symfony/asset": "^5.3|^6.0",
"symfony/browser-kit": "^4.4|^5.0|^6.0",
"symfony/console": "^5.2|^6.0",
"symfony/console": "^5.4|^6.0",
Copy link
Member

Choose a reason for hiding this comment

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

the conflict line should be updated too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Jul 2, 2021
@yoannrenard yoannrenard force-pushed the add_commandIsSuccesful_helper branch 2 times, most recently from 4d7badc to 434e3d2 Compare July 2, 2021 12:22
@chalasr chalasr force-pushed the add_commandIsSuccesful_helper branch from 434e3d2 to 6221527 Compare July 3, 2021 17:16
@chalasr
Copy link
Member

chalasr commented Jul 3, 2021

Thank you @yoannrenard.

@chalasr chalasr merged commit cdcf696 into symfony:5.4 Jul 3, 2021
@yoannrenard yoannrenard deleted the add_commandIsSuccesful_helper branch July 4, 2021 06:48
@OskarStark OskarStark mentioned this pull request Jul 15, 2021
Merged
This was referenced Nov 5, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus approved these changes

@chalasr chalasr chalasr approved these changes

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Feature Status: Needs Review
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@yoannrenard @carsonbot @xabbuh @chalasr @nicolas-grekas @derrabus