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

Introduce TestRunAwareTestListener #3381

Closed
wants to merge 1 commit into from
Closed

Introduce TestRunAwareTestListener #3381

wants to merge 1 commit into from

Conversation

rpkamp
Copy link
Contributor

@rpkamp rpkamp commented Nov 4, 2018

Adds startTestRun and endTestRun hooks to TestListener so implementing
classes don't have to check whether a startTestSuite call is the first
one, plus it allows implementers to do something after the very last
test, which is usefull for implementers that want to buffer data and
only output once everything is done.

Adds startTestRun and endTestRun hooks to TestListener so implementing
classes don't have to check whether a startTestSuite call is the first
one, plus it allows implementers to do something after the very last
test, which is usefull for implementers that want to buffer data and
only output once everything is done.
@codecov
Copy link

codecov bot commented Nov 4, 2018

Codecov Report

Merging #3381 into master will increase coverage by 0.01%.
The diff coverage is 96.96%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3381      +/-   ##
============================================
+ Coverage     82.95%   82.97%   +0.01%     
- Complexity     3569     3580      +11     
============================================
  Files           143      143              
  Lines          9499     9513      +14     
============================================
+ Hits           7880     7893      +13     
- Misses         1619     1620       +1
Impacted Files Coverage Δ Complexity Δ
src/Runner/Hook/TestListenerAdapter.php 100% <100%> (ø) 34 <6> (+6) ⬆️
src/Framework/TestResult.php 75.97% <100%> (+0.47%) 173 <6> (+5) ⬆️
src/TextUI/TestRunner.php 68.46% <100%> (-0.3%) 284 <0> (-2)
src/TextUI/ResultPrinter.php 97.35% <100%> (+0.02%) 80 <2> (+1) ⬆️
src/Util/Log/TeamCity.php 52.76% <85.71%> (-0.04%) 63 <2> (+1)

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 bcbb5e0...f226210. Read the comment docs.

@sebastianbergmann
Copy link
Owner

BeforeFirstTestHook and AfterLastTestHook already exist. Or am I missing something?

@sebastianbergmann
Copy link
Owner

Maybe I should have documented the fact that the legacy TestListener interface that contains way too many methods will eventually go away.

Units of code that want to observe test execution should use the new TestHook interfaces. This is done by writing a class that implements one or more TestHook interfaces and registering it as a test runner extension.

@rpkamp
Copy link
Contributor Author

rpkamp commented Nov 4, 2018

Okay, so basically I've made the lagacy code forward compatible? 😄

Okay, if the TestListener needs to go -which I agree would be a good thing, as it doesn't really adhere to the Interface Seggregation Principle- then what happens to the printers?

The printers are currently all implemented as TestListeners, but should they become extensions instead?

The whole point of this PR was to get a signal in the CliTestDoxPrinter when the last test had run so we could output any buffered content, required for #3380; my idea was to create a BufferedTestListener as a decorator for other TestListeners that would order and output all data at the end when all test are done. This decorator could then be used general purposes for all kinds of printers that need sorted output.

Also, I don't like using printResults as a signal to output the buffered content, as printResult is not part of any interface, so it relies on duck typing.

The interfaces exist, now it's a matter of figuring out how to best use them to solve this, I think.

@epdenouden
Copy link
Contributor

epdenouden commented Nov 5, 2018

@rpkamp Perhaps the caching listener (or listening cache? :) can be of some inspiration too:

final class ResultCacheExtension implements AfterSuccessfulTestHook, AfterSkippedTestHook, AfterRiskyTestHook, AfterIncompleteTestHook, AfterTestErrorHook, AfterTestWarningHook, AfterTestFailureHook, AfterLastTestHook

It started off as a version of the old listener-printer structure, then @sebastianbergmann asked to refactor it more cleanly before merge. Works like a charm.

@rpkamp
Copy link
Contributor Author

rpkamp commented Nov 5, 2018

@epdenouden yes, that looks good.

However, since that's an extension and the printer is a TestListener I'd need an adapter. I'd like a decorator instead, but for that to work the printer would need to be an extension too 🙂

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.

3 participants