Diagnostic events in tests #1209

dmdashenkov · 2019-12-02T12:52:45Z

`Dashboard`

When testing a Spine-based system with BlackBoxBoundedContext, it is important to be aware of all the failures which happen backstage. For this, we introduce a new system event subscriber — Dashboard. A Dashboard logs all the diagnostic events which occur in the Bounded Context under test. The only way to turn the Dashboard off is via MuteLogging aspect.

Duplicate signal events

Previously, CannotDispatchDuplicateEvent and CannotDispatchDuplicateCommand events did not provide any info on the type of the duplicate signal. Now, instead of providing info about the ID, the type info is also included in the events.

Fixes #1207.

codecov · 2019-12-02T13:05:35Z

Codecov Report

Merging #1209 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1209      +/-   ##
============================================
+ Coverage     91.46%   91.48%   +0.02%     
- Complexity     4260     4269       +9     
============================================
  Files           546      547       +1     
  Lines         13289    13346      +57     
  Branches        758      759       +1     
============================================
+ Hits          12155    12210      +55     
  Misses          895      895              
- Partials        239      241       +2

dmdashenkov · 2019-12-02T14:12:50Z

@alexander-yevsyukov, PTAL.

alexander-yevsyukov

I like the design and impl. in general. But I have some questions and comments. Please have a look. Let's talk if you need more info on why I think some of the changes are questionable.

alexander-yevsyukov · 2019-12-02T17:35:53Z

.idea/codeStyles/Project.xml

@@ -30,6 +30,12 @@
    </XML>
    <codeStyleSettings language="Dart">
      <option name="RIGHT_MARGIN" value="100" />
+      <option name="KEEP_BLANK_LINES_IN_CODE" value="1" />


We keep tossing these settings back and forth by different people. I guess we need to standardize either to have them or not have them.

Those come from config. It's strange that they get deleted sometimes. I vote for leaving them.

alexander-yevsyukov · 2019-12-02T17:39:29Z

server/src/main/java/io/spine/server/delivery/CommandEndpoint.java

@@ -33,6 +33,6 @@
    @Override
    default void onDuplicate(I target, CommandEnvelope envelope) {
        repository().lifecycleOf(target)
-                    .onDuplicateCommand(envelope.id());
+                    .onDuplicateCommand(envelope.messageId());


Is there any other ID we have in relation to an envelope? Why do we need a longer name in this context?

alexander-yevsyukov · 2019-12-02T17:41:56Z

server/src/main/java/io/spine/server/entity/EntityLifecycle.java

@@ -367,22 +367,26 @@ public final void onDispatchingFailed(MessageId dispatchedSignal, Error error) {
        }
    }

-    public void onDuplicateEvent(EventId eventId) {
+    public void onDuplicateEvent(MessageId eventId) {


This looks suspicious. I would understand having the method renamed to onDuplicatedMessage() retaining the parameter type of EventId, so that we can have onDuplicateMessage(CommandId) below. But this generalization looks a bit strange. Why do we need it?

alexander-yevsyukov · 2019-12-02T17:43:41Z

server/src/main/java/io/spine/server/entity/EntityLifecycle.java

        CannotDispatchDuplicateEvent event = CannotDispatchDuplicateEvent
                .newBuilder()
                .setEntity(entityId)
-                .setEvent(eventId)
+                .setEvent(eventId.asEventId())
+                .setDuplicateEvent(eventId)


Why do we need to speak duplicated again? We already said that in the name of the rejection.

Also, why do we set general type MessageId while we know the specific type (and say it in the name of the field)?

alexander-yevsyukov · 2019-12-02T17:45:48Z

server/src/main/proto/spine/system/server/diagnostic_events.proto

+    //
+    core.CommandId command = 2 [deprecated = true];
+
+    core.MessageId duplicate_command = 3;


It's not clear why do we need a general type while we say command in the field name. Please explain.

MessageId also contains the type URL of the command, while CommandId does not. We don't lose any information when using MessageId here.
The same applies to EventId in CannotDispatchDuplicateEvent.

alexander-yevsyukov · 2019-12-02T17:46:22Z

server/src/main/proto/spine/system/server/diagnostic_events.proto

+    //
+    core.EventId event = 2 [deprecated = true];
+
+    core.MessageId duplicate_event = 3;


It's not clear why do we have a more general type MessageId while we speak specific type event in the field name. Please explain.

alexander-yevsyukov · 2019-12-02T17:51:46Z

testutil-server/src/main/java/io/spine/testing/server/blackbox/Dashboard.java

+ *
+ * <p>Logs all the received diagnostic events with a meaningful message.
+ */
+final class Dashboard


I like the design, but I'm not sure this is the best name. I would keep such a general (and UI-related term) for a bigger thing. Speaking something about “diagnostics” in the name would look more natural.

alexander-yevsyukov · 2019-12-02T17:52:26Z

testutil-server/src/main/java/io/spine/testing/server/blackbox/Dashboard.java

+        String typeUrl = event.getEntity()
+                              .getTypeUrl();
+        String idAsString = Identifier.toString(event.getEntity().getId());
+        log(event, "Entity state (ID: %s, type: %s) is invalid.", idAsString, typeUrl);


Please add backticks to code-related things in the messages here and below.

alexander-yevsyukov · 2019-12-02T18:03:25Z

testutil-server/src/main/java/io/spine/testing/server/blackbox/Dashboard.java

+
+    @Subscribe
+    void on(HandlerFailedUnexpectedly event) {
+        log(event, "Signal %s could not be handled by %s:%n%s",


As we're putting messages on new line (and understandably not having a period in the format) we need to separate this message from the next one on the log.

Options that I see here:

Have some indentation after %n.

Have another %n at the end, so that the whole log record on an event looks as a clearly separated block.

Without it the message which starts at the new line could be confused with a start of a new log record. It's easy to miss the colon at the end of the previous line, especially if the log line is long and full of code-related details.

alexander-yevsyukov

LGTM

dmdashenkov added 9 commits December 2, 2019 12:23

Add Dashboard event subscriber

41b9abc

Make Dashboard a singleton

2d07f33

Update config

b4be536

Make subscriber methods package-private

a011a8a

Mute unnecessary logging

5b1257f

Add Dashboard to the black box BC

527e7a9

More doc for Dashboard

e099d7a

Use MessageId for system events signifying duplicate signals

46a8ca2

More info in error message in Dashboard

009c18f

dmdashenkov added tech debt tests labels Dec 2, 2019

dmdashenkov self-assigned this Dec 2, 2019

Bump version to 1.2.7

8277c16

dmdashenkov added 3 commits December 2, 2019 15:06

Update reports

5828965

Add tests for Dashboard

1d51d4f

Fix tests for Dashboard

985e597

dmdashenkov marked this pull request as ready for review December 2, 2019 14:12

dmdashenkov requested a review from alexander-yevsyukov December 2, 2019 14:12

alexander-yevsyukov requested changes Dec 2, 2019

View reviewed changes

dmdashenkov added 5 commits December 3, 2019 16:48

Improve error messages

6374da1

Rename Dashboard to DiagnosticLog

0e23c25

Remove jumpy configuration

4550513

Make EntityLifecycle method params more type-strict

8cba02a

Update config

1904344

alexander-yevsyukov approved these changes Dec 3, 2019

View reviewed changes

dmdashenkov merged commit a063012 into master Dec 3, 2019

dmdashenkov deleted the visible-diagnostic-events branch December 3, 2019 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diagnostic events in tests #1209

Diagnostic events in tests #1209

dmdashenkov commented Dec 2, 2019 •

edited

Loading

codecov bot commented Dec 2, 2019 •

edited

Loading

dmdashenkov commented Dec 2, 2019

alexander-yevsyukov left a comment

alexander-yevsyukov Dec 2, 2019

dmdashenkov Dec 3, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov Dec 2, 2019

dmdashenkov Dec 3, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov Dec 2, 2019

alexander-yevsyukov left a comment

Diagnostic events in tests #1209

Diagnostic events in tests #1209

Conversation

dmdashenkov commented Dec 2, 2019 • edited Loading

Dashboard

Duplicate signal events

codecov bot commented Dec 2, 2019 • edited Loading

Codecov Report

dmdashenkov commented Dec 2, 2019

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

dmdashenkov commented Dec 2, 2019 •

edited

Loading

`Dashboard`

codecov bot commented Dec 2, 2019 •

edited

Loading