Improve Event API

[exodist]

The current event API looks like this:

# Contextual info
if ($trace = $e->trace) {
    $pid     = $trace->pid;        # Process ID
    $tid     = $trace->tid;        # Thread ID
    $cid     = $trace->cid;        # Context ID
    $package = $trace->package;    # Package where context was made
    $file    = $trace->file;       # File where context was made
    $line    = $trace->line;       # Line where context was made
    $subname = $trace->subname;    # name of subroutine that made context (may be __ANON__)

    # Only works if trace is defined
    $related = $e->related($e2);
}

# Subtest related
$subtest_depth = $e->nested;        # integer
$subtest_id    = $e->in_subtest;    # Id (string) of the subtest this test is insite, may be undef
$subtest_id    = $e->subtest_id;    # Id (string) of the subtest if this event is the final result of a subtest

# Plugin/Tool Meta Data
# Usually 'key' should be the name of the plugin that sets it IE 'My::Plugin'.
$val = $e->get_meta($key);
$val = $e->set_meta($key, $val);
$val = $e->meta($key, $default);
$val = $e->delete_meta($key);

# Routing through the hub
$exit   = $e->terminate;            # undef normally, define (set to exit code) if the event causes an immediete exit.
$global = $e->global;               # Used to say that an event should be routed to all processes/threads/hubs.

# Methods that effect hub state
$e->callback($hub);                 # Called by the hub during event processing just before the event hits the formatter.
$failed    = $e->causes_fail;       # True if the event means there has been a failure
$inc_count = $e->increments_count;  # True if the event should bump the test count (an assertion was made)
($max, $directive, $reason) = $e->sets_plan;    # If an event sets the plan, this explains the plan.

# Intended for renderers
$diag       = $e->diagnostics;                  # True if the event has diagnostics in (IE should always be shown, even in a renderers summary mode, in TAP this means STDERR)
$no_display = $e->no_display;                   # The event is not intended to be rendered by the formatter (but the formatter still gets it)
$summary    = $e->summary;                      # A human readable summary of the event.

# Other 
$json = $e->TO_JSON;

We need to improve this, some of these methods are poorly named/designed, and not all the info we want to convey is included.

EDITED Original suggestion was scrapped.

[exodist]

I could also see some people writing complex event subclasses and wanting to use Moose, so maybe we do not want to implement a 'does' method? Or do we want to be simple and assume nobody is going to white such a complex event (the way events are sent VIA IPC may limit it already)

Personally I think that event subclasses should be as simple as possible.

[exodist]

To be clear for anyone who reads this, we are not looking to use Moose in Test2/Test-Simple, and we are also not looking to re-implement the stuff Moose does. In this case we are using the generic term 'role' as a contract of functionality an event may implement.

[exodist]

I personally am heistant to use a 'does' method, or using 'does' in the shorthand names. I would suggest we drop does() and use ```made...``` so:

$e->made_assertion();
$e->made_plan();
$e->made_info();
# etc...

[shadowcat-mst]

I think at this point it can be argued that does() has an expected meaning in perl code just like isa() does, and that overloading that meaning by borrowing the name is going to confuse the heck out of people. I quite like 'has_assertion', 'has_plan' and 'has_info', and given the three need to be handled differently I'm not sure you need a generic version that takes a string.

[exodist]

updated to drop 'does' and use 'has_xxx'

[exodist]

We might also really simplify this:

    $assert = $e->assertion;
    $plan    = $e->plan;
    $msg1 = $e->info;
    $mdg2 = $e->diag;

$assert and $plan can be undef if they do not apply, $plan will be 0->maxint if it does apply, and $assert will be 0 or 1 if it applies. $msg1 and $msg2 will be undef if there is none, and a defined value if there is one.

[schwern]

I think the single most important thing we must do before going over technical details and discussing changing the event API is to write a second, serious event handler. Probably a formatter for Jenkins or other JUnit style as that's also very useful. We'll learn a lot. Use that to drive changes to he Event model.

I've always had a hard time finding a spec for JUnit test output, here's the best I can find.

[autarch]

@2shortplanks is working (last I knew) on a formatter for TeamCity.

[exodist]

I am also planning to take the formatter Test2::Harness ultimately uses and make it into a real Test2 formatter.

[exodist]

@schwern, this ticket is not a "fix this now" ticket. it is a "this needs to be better, lets take the time to discuss and do other necessary steps" ticket.

[2shortplanks]

I think some discussion about my real world experience with TeamCity might be useful here.

TeamCity has its own text format and own events that it needs to render. The task I have in front of me is to take a stream of events that Test2 is generating and print out the corresponding TeamCity events. This is made significantly harder by the fact that TeamCity does not have a one-to-one mapping to Test2 events. For example, it has the concept of an event starting a test, and an event ending a test, and optionally further events between those two event that fail the test and create diagnostics associated with that test. Contrast this with Test2 that has a single event that indicates a test has simply succeeded or failed, and optionally separate events that subsequently follow containing diagnostic messages that presumably (but not necessarily) are associated with the previous success/failure event.

You can see in order to make this work I'm starting to hardcode assumptions and best-guesses into the system to try and get these two things to speak a common language. This is not good.

The chief problem I'm seeing here is that there's very little way to express connections between events in this system. The events mimic the limitations of Test2 itself; Just because Test2 has no current way of expressing that a diagnostic message is associated with a current test doesn't mean the event should be limited in this way. Even if I hack our Test::Class::Moose test suite to send a "close" event whenever a new test start and whenever we get to the end of a test_* method, my renderer is going to have to keep a heck of lot of state that I'd rather the event handle for me.

[autarch]

@2shortplanks - doesn't the cid attribute in the recent Test2 release answer the "is this associated" question?

[exodist]

I have created the new_even_api branch. I am planning to start by porting the Test2::Harness renderer to be a proper formatter. As I do I will note things that are hard that can be improved by a better event api. I will likely also implement base ideas from the above proposal, but I will not polish them as I expect writing formatters will result in a lot of ideas/changes.

I suggest all of us who are trying to write formatters work with this branch

[exodist]

As expected, once you actually try to do all this (formatter, new api, etc) you find all kinds of stuff that makes you quickly change plans... here is what I have for tonight: https://github.com/Test-More/test-more/tree/new_event_api looking at the event base class you can see I ultimately deviated a LOT from what I have above.

[exodist]

btw, if you have not figured it out yet, my development style is this:

• Plan something a little bit, then jump in
• Find all the pain the idea causes by trying it out
• Adjust idea and plan until making the change is not painful
• Continue to adjust until it feels really nice to use.
• Check back-compat, and impact, adjust until both are in a good place
• Unstable release for a while
• Go stable

By this plan it will be months before I have anything I would actually consider inflicting on anyone.

[exodist]

https://github.com/Test-More/test-more/pull/760 is where I am working on this now. Very much has changed from the original at the top.

[exodist]

This ticket and the final branch I have for this have diverged enough to close this and create a new ticket: #766