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

Output handler refactoring #345

Closed
wants to merge 1 commit into from

Conversation

milo
Copy link
Member

@milo milo commented Dec 19, 2016

This is how I image solution for #326. So, the main purpose is to provide more information for OutputHandler.

This is a WIP concept with many TODOs to solve. One problem is, that the new Test class shares many common properties with Job and I'm not sure if it's OK.

@dg dg force-pushed the master branch 2 times, most recently from 715ed4e to 334dc33 Compare January 4, 2017 14:23
*/
public function withResult($result, $message)
{
# TODO: Vyjimku, pokud je test failed?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd throw an exception if the test already has any result

class Test
{
const
NO_RESULT = 0, # TODO: lepsi jmeno
Copy link
Contributor

Choose a reason for hiding this comment

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

what about PENDING?

@@ -95,7 +90,7 @@ public function run()
$handler->begin();
Copy link
Contributor

@jiripudil jiripudil Apr 13, 2017

Choose a reason for hiding this comment

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

I'd love to be able to get $this->jobCount in here, but that would mean moving this call after $this->findTests() in which results may already be sent to output. They'd have to be buffered somehow. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

More precisely, the most perfect scenario would be if I could get an array of all Tests so that the output handler could process them (similarly to what I did in #335). It would make sense to me if findTests created and actually returned found Tests, and the while loop iterated over them instead of Jobs. But that would mean that Tests would somehow have to be aware of their respective Jobs, which is something you didn't quite like

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to outline one possible (and sensible to me) solution in milo#1

/**
* @return array
*/
public function getJobArguments() # TODO: tohle sem asi nepatri
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the arguments are abstracted in an array, maybe Job should just call getArguments() and do this formatting itself? It feels coupled to the way Jobs are executed.

@jiripudil jiripudil mentioned this pull request Apr 17, 2017
1 task
@jkuchar
Copy link
Contributor

jkuchar commented Apr 18, 2017

In original #334 there was Test which contains Job, here it is in reverse. What is the reasoning behind that? #345 feels more natural from test executor perspective. From user perspective original approach from #334 feels more natural. Is that what you had on mind?

@milo milo force-pushed the output-handler-refactoring branch from 9634df4 to 61c4ca5 Compare May 2, 2017 16:02
@milo milo force-pushed the output-handler-refactoring branch from 61c4ca5 to c56cab9 Compare May 2, 2017 16:07
@milo
Copy link
Member Author

milo commented May 2, 2017

The first part is merged a34637b, 65ea0f8.

So, this PR is about OutputHandler only now. I'm thinking about BC. I have no idea, how many custom OutputHandlers exist. Maybe introduce a new interface, it would be without BC.

@milo
Copy link
Member Author

milo commented May 2, 2017

@jiripudil Thanks for review.
@jkuchar From my point of view, is better that the Test is only independent wrapper and Job fills it.

@jiripudil
Copy link
Contributor

@milo You're welcome, sorry it took me so long.

What do you think of the idea behind milo#1 ? It makes sense to me because some output handlers might use the information about the tests before printing the results.

I know it's a BC break, as is the rest of this pull, but since it targets 2.0, I think it's justifiable. Although a lot of the internal code has changed, the changes to the public API (OutputHandler) are minor and easy to follow.

I know @mrtnzlml has written a custom handler. As for others, maybe you could ask on Twitter?

jiripudil added a commit to nette-intellij/intellij-nette-tester that referenced this pull request May 21, 2017
The output handler is a deliberate step back: it is very simple as it just prints the results without grouping them in any way. But, on a might brighter note, this approach works with pretty much any version of Nette Tester 🎉 (a more intelligent output handler implementation waits for the resolution of nette/tester#345)

Breaking change: a user-defined setup script, previously configured via --setup entry in "Tester options", must now be provided in a separate "User setup script" option in the Run Configuration dialog, otherwise tests and/or the plugin might not work as expected.
@jiripudil
Copy link
Contributor

btw it seems that the OutputHandler API has already (potentially) broken BC by moving the result constants from Runner to Test, see jiripudil/intellij-nette-tester#30 🤔

@dg dg force-pushed the master branch 3 times, most recently from c7e8a12 to df5e87f Compare July 24, 2017 14:21
@milo milo mentioned this pull request Jul 31, 2017
@milo
Copy link
Member Author

milo commented Aug 13, 2017

Closing in favour of #370

@milo milo closed this Aug 13, 2017
@milo milo deleted the output-handler-refactoring branch August 15, 2017 07:17
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.

None yet

3 participants