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

Working draft for Event subsystem #1

Closed
wants to merge 675 commits into from
Closed

Working draft for Event subsystem #1

wants to merge 675 commits into from

Conversation

localheinz
Copy link
Collaborator

@localheinz localheinz commented Oct 5, 2019

This PR

  • proposes an even sub-system for phpunit

Build Status

CI


public function is(Type $other): bool
{
return $other->asString() === $this->asString();
Copy link

Choose a reason for hiding this comment

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

This seems prone to collision with other types.
Maybe change to get_class($other) === get_class($this) && $other->asString() === $this->asString()?

Note that in PHP the object has access to the $name property of $other since it's in the same class, so this would also work:

return get_class($other) === get_class($this) && $other->name === $this->name;

Copy link
Owner

Choose a reason for hiding this comment

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

As this class will vanish (see below), this point is probably mute.

Regarding property access: Yes, we are aware of that but I consider it bad software design. The asString is part of the public API, the existence of the property name is a (private) implementation detail.

src/Event/Types.php Outdated Show resolved Hide resolved
{
public static function type(Type $type): self
{
return new self(\sprintf('Type "%s" not supported', $type->asString()));
Copy link

Choose a reason for hiding this comment

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

I would add the class name of the subscriber here to create a bit of context: Event subscriber "Foo\Bar\BazSubscriber" does not support events of type "blah"

Copy link
Owner

Choose a reason for hiding this comment

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

Good idea.

Even though this should technically be almost impossible to ever trigger, given that the dispatcher uses the same method to find out what types of events to send to this subscriber.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, thank you, @rpkamp!

src/Event/Event.php Outdated Show resolved Hide resolved
@rpkamp
Copy link

rpkamp commented Oct 6, 2019

Really awesome that you're implementing this 👍 🚀

@localheinz localheinz self-assigned this Oct 8, 2019
src/TextUI/Command.php Outdated Show resolved Hide resolved
src/Framework/TestCase.php Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #1 into master will increase coverage by 0.61%.
The diff coverage is 91.39%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #1      +/-   ##
============================================
+ Coverage     83.56%   84.18%   +0.61%     
- Complexity     4501     4733     +232     
============================================
  Files           218      260      +42     
  Lines         11030    11745     +715     
============================================
+ Hits           9217     9887     +670     
- Misses         1813     1858      +45
Impacted Files Coverage Δ Complexity Δ
src/TextUI/Configuration/PHPUnit/Extension.php 100% <ø> (+29.78%) 6 <0> (-11) ⬇️
src/Event/GlobalState/Restored.php 100% <100%> (ø) 2 <2> (?)
src/Event/Test/TearDownFinished.php 100% <100%> (ø) 2 <2> (?)
src/Event/GlobalState/Captured.php 100% <100%> (ø) 2 <2> (?)
src/Event/Test/RunFailed.php 100% <100%> (ø) 7 <7> (?)
src/Event/Telemetry/MemoryUsage.php 100% <100%> (ø) 4 <4> (?)
src/Event/Facade.php 100% <100%> (ø) 8 <8> (?)
src/Event/Test/RunWarning.php 100% <100%> (ø) 7 <7> (?)
src/Event/Comparator/Registered.php 100% <100%> (ø) 2 <2> (?)
src/Event/Telemetry/Snapshot.php 100% <100%> (ø) 4 <4> (?)
... and 83 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00e1cf3...3e7e63f. Read the comment docs.

localheinz and others added 27 commits May 21, 2021 14:45
Fixes #68.

Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Fixes #32.

Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Remove redundant formatter

Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
Co-authored-by: Andreas Möller <am@localheinz.com>
Co-authored-by: Arne Blankerts <Arne@Blankerts.de>
@theseer
Copy link
Owner

theseer commented May 21, 2021

This PR is closed as it was merged by other means.

@theseer theseer closed this May 21, 2021
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.

7 participants