Page MenuHomePhabricator

When using the Herald test console on a transactional object, guess a reasonable set of transactions to simulate
ClosedPublic

Authored by epriestley on May 13 2019, 5:15 PM.

Details

Summary

Depends on D20518. Ref T13283. When you use the "Test Console" in Herald to test rules, pass the most recent group of transactions to the Adapter.

This will make it easier to test rules that depend on edit state, since you can make the type of edit you're trying to test and then use the Test Console to actually test if it matches in the way you expect.

The transactions we select may not be exactly the group of transactions that most recently applied. For example, if you make edits A, B, and C in rapid succession and then run the Test Console on the object, it may select "A + B + C" as a transaction group. But we'll show you what we selected and this is basically sane/reasonable and should be fine.

(Eventually, we could include a separate "transaction group ID" on transactions if we want to get this selection to match exactly.)

Test Plan

Ran the Test Console on various objects, saw sensible transaction lists in the transcripts.

Diff Detail

Repository
rP Phabricator
Branch
herald2
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 22785
Build 31257: Run Core Tests
Build 31256: arc lint + arc unit

Event Timeline

epriestley created this revision.May 13 2019, 5:15 PM
epriestley requested review of this revision.May 13 2019, 5:17 PM
amckinley accepted this revision.May 16 2019, 1:12 PM
amckinley added inline comments.
src/applications/herald/controller/HeraldTestConsoleController.php
283

Should we check if we've grouped all 100 of the fetched transactions together and start paging if we did? Or maybe just show a little "this simulation returned more than 100 recent transactions and is therefore only an approximate simulation" notice?

This revision is now accepted and ready to land.May 16 2019, 1:12 PM

Since shouldDisplayGroupWith() includes a 2-minute cutoff and requires transactions have the same author, and we merge transactions that are part of the same edit before applying them (so if you submit 100 "edit title" transactions at the same time, we only actually apply the last one, since none of the others have any effect) I think you could only generate a group with 100+ transactions by writing a script that repeatedly edited the same field to different values very quickly. This seems very unlikely, even given the great ambition I ascribe to our users.

If a user writes, say, one title edit every second for 5 minutes ("a" -> "aa" -> "aaa" and so on), then runs the test console, we'll currently build a group with the 100 most recent title edits.

If we keep paging, we'll build a group with the 300 most recent title edits (5 minutes * 60 edits per minute).

The actual evaluation that Herald originally performed was 300 different groups with 1 transaction each, so I think neither "the 100 most recent edits" or "the 600 most recent edits" are particularly better representations of the original behavior.

To show a warning, we'd also have to pass the state through the Adapter to the Transcript, since the page that renders the transactions is on the other side of the dry run.

I think a cleaner improvement here is probably to generate a random unique "Transaction Group ID" and add it to all the transactions on edit, then use that instead of the "display grouping" rules to select a group (i.e., select all the transactions which have the same "group ID" as the most recent transaction, indicating that they truly applied in the same edit). Then, you could generate a list of 100+ transactions only by adding ~90+ custom fields and editing every field at once, which is hopefully pathological (and we could page our way through that, and throw some kind of "you are a bad person" exception if you build a group with 1,000+ transactions).

In all these cases, I suspect the actual goal ("understand how Herald rules work") is probably accomplished about equally well.

Upshot: this could be better, but I think it only affects cases which require truly impressive ambition to create. If/when we do improve it, I'd like to try annotating transactions that we apply together with a shared ID so we can authentically reconstruct the most recent transaction group without using "display grouping" rules, which should be very good but are not exactly the same in all cases.

Doing this right (or, at least, more right) is probably about 20 lines of code and would make transaction.search a little more useful, so I may just take a stab at it at the end of this series.