8000 Split and clean up tests by localheinz · Pull Request #49286 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Split and clean up tests #49286

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
Feb 10, 2023
Merged

Split and clean up tests #49286

merged 1 commit into from
Feb 10, 2023

Conversation

localheinz
Copy link
Contributor
@localheinz localheinz commented Feb 7, 2023 •
edited
Loading
Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

This pull request

  • splits a test and removes a data provider to avoid creating test doubles in data providers

Follows #49244.

💁‍♂️ This follows a brief discussion on Twitter (see https://twitter.com/VotrubaT/status/1622995017756291074).

I do not expect this to be merged, but in my opinion, creating

  • test doubles
  • the system under test (not the case here, but I have observed it elsewhere)

in data providers are anti-patterns.

Other than that, I have no idea what we are testing here.

@carsonbot carsonbot added this to the 5.4 milestone Feb 7, 2023
@localheinz localheinz force-pushed the fix/test-double branch 2 times, most recently from bcacdea to 2e80ffd Compare February 7, 2023 18:26
@localheinz localheinz changed the title Fix: Split and clean up tests [SecurityBundle] Split and clean up tests Feb 7, 2023
@localheinz localheinz force-pushed the fix/test-double branch 4 times, most recently from 34ca5f2 to 55e5d08 Compare February 7, 2023 18:37
@alfredbez
Copy link

I'm pretty sure that you forgot to add the words "in dataproviders" in this paragraph:

[...] in my opinion, creating

  • test doubles
  • the system under test (not the case here, but I have observed it elsewhere)

are anti-patterns.

@localheinz
Copy link
Contributor Author

Thank you, @alfredbez - adjusted the description!

@carsonbot carsonbot changed the title [SecurityBundle] Split and clean up tests Split and clean up tests Feb 8, 2023
@nicolas-grekas
Copy link
Member

Thank you @localheinz.

@nicolas-grekas nicolas-grekas merged commit 089ec9d into symfony:5.4 Feb 10, 2023
@localheinz localheinz deleted the fix/test-double branch February 10, 2023 14:20
@localheinz
Copy link
Contributor Author

Thank you, @alfredbez, @nicolas-grekas, and @OskarStark!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark approved these changes

@wouterj wouterj Awaiting requested review from wouterj

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

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

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

Successfully merging this pull request may close these issues.
5 participants
@localheinz @alfredbez @nicolas-grekas @OskarStark @carsonbot
0