Skip to content

[PHPunitBridge] Count @expectedDeprecation as an assertion #21896

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
Mar 6, 2017

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Mar 6, 2017

Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

By adding addToAssertionCount(), the @expectedDeprecation annotation is recognized as an assertion by PHPUnit. This means PHPUnit will not report the test as risky because it does not contain any assertion.

@xabbuh
Copy link
Member

xabbuh commented Mar 6, 2017

see #21786 and #21828 :)

@fabpot
Copy link
Member

fabpot commented Mar 6, 2017

@xabbuh This can be closed then, right?

@xabbuh
Copy link
Member

xabbuh commented Mar 6, 2017

@fabpot We could discuss whether #21828 should have been merged into 3.2 as a bugfix (I don't think so, but others may disagree).

@wouterj
Copy link
Member Author

wouterj commented Mar 6, 2017

Yes, except that #21828 is merged as a feature. However, I consider it a bug fix and thus it has to be backported to 3.2. Imo, counting the expectedDeprecation assertion not as an assertion is a bug. PHPunit 6's default enabled risky test checker reveals the bug (but the bug is there in all PHPunit versions).

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.

👍 for 3.2 then

@xabbuh
Copy link
Member

xabbuh commented Mar 6, 2017

Fair enough, but note that you will probably check first that the test is neither skipped nor incomplete (see how #21828 was implemented).

@wouterj
Copy link
Member Author

wouterj commented Mar 6, 2017

Yeah, I think if it's decided to backport this to 3.2, the #21828 PR should simply be backported (and this PR should just be closed).

@fabpot
Copy link
Member

fabpot commented Mar 6, 2017

Except the trait does not exist in 3.2 IIRC, so this should be done here.

@wouterj
Copy link
Member Author

wouterj commented Mar 6, 2017

Ah, my bad. Updated the PR to match the code added in 3.3.

Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

👍

@fabpot
Copy link
Member

fabpot commented Mar 6, 2017

Thank you @wouterj.

@fabpot fabpot merged commit ba5c0f4 into symfony:3.2 Mar 6, 2017
fabpot added a commit that referenced this pull request Mar 6, 2017
… (wouterj)

This PR was merged into the 3.2 branch.

Discussion
----------

[PHPunitBridge] Count @expectedDeprecation as an assertion

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

By adding `addToAssertionCount()`, the `@expectedDeprecation` annotation is recognized as an assertion by PHPUnit. This means PHPUnit will not report the test as risky because it does not contain any assertion.

Commits
-------

ba5c0f4 Count @expectedDeprecation as an assertion
@wouterj wouterj deleted the patch-17 branch March 6, 2017 16:33
@fabpot fabpot mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants