Move most skips to single file, expand API to handle conditions

[katcharov]

• Moves skips to a single file.
• Cleans up skips. Adds explicit files, or full test names, to disambiguate. Expands on reasons for skips, or associates particular Jira tickets, based on blame and associated tickets/PRs. Some skips are still incomplete.
• Expands API. Adds a mechanism to apply certain "modifiers" (ignoring extra events) for tests, instead of skipping them. This removes custom logic from test subclasses.
• Leaves one unified test with custom logic, ClientSideOperationTimeoutTest.

[NathanQingyangXu]

• better renaming to avoid confusion
• consider refactoring modifier out of skip class

[NathanQingyangXu]

technically speaking, modifier mechanism doesn't belong to skips scope (as anticipated by the class name and its implementation) and adding it here seems unnatural and confusing.
Could we separate its concern into its own class? Then we could simplify the logic in this class a lot.

[katcharov]

Renamed class

[NathanQingyangXu]

both shouldApply and condition naming leave room of improvement. Currently the naming would not help too much on mastering the gist without reading code very carefully.

maybe condition and shouldApply could both be changed to precondition (one example only) ? Then code reader might be less hard-pressed.

[katcharov]

Renamed

[NathanQingyangXu]

If the first two parameters are File system related, maybe using File or Path and go about comparison based on those classes might be more robust and a / dir suffix would not lead to unexpected behaviour.

[katcharov]

I think we should not allow both "/path" and "path". Using non-strings would increase boilerplate.

[NathanQingyangXu]

the core of the coding logic in this whole class. It is still not intuitive and takes quite some scratching hairs to figure out. Some complexities include:

1. ensure condition is the very first fluent usage, if used;
2. matching includes both 'skip matching' and 'modifier matching';
3. matching should be the step AFTER condition, so matchWasPerformed variable is introduced.

From my understanding, the logic is a little bit too intertwining and unnatural. I think each of the three above concerns could have natural and elegant solution:

1. if condition is the very first input if provided, we could keep it from fluent usage scope and allocate a constructor parameter for it; then no trouble at all and no further clumsy coding is needed
2. I highly recommend refactoring modifier to its own class; are they only relevant to reactive testing? Then maybe reactive unified tester is a better location;
3. the above code would be simplified so much. The confusing shouldSkip and matchWasPerformed could be deleted.

[katcharov]

For 1, the constructors are not used directly. I am not sure how to interpret this, except to produce multiple methods (skipJira(string) and skipJira(string, condition)), but then the readability suffers when the API is used.

For 2, I am not sure if the suggestion is to move invocations to a separate file (I think it is better to show how we are handling a specific set of tests in one place), or to copy/extend TestApplicator (I would need to better understand the benefits of this).

For 3, I think this focuses on the internals, rather than the API surface, which is less important. For any API, the most important thing (in my opinion) is how easy it is to read, then how easy it is to use, and only then how easy it is to modify the internals (however, I do not see these internals as particularly complex).

[NathanQingyangXu]

skipTodo is a little bit confusing. sounds like skipping todo or no-op.

[katcharov]

Renamed

[NathanQingyangXu]

the naming of skip is not accurate. Guess the intention is reason or suchlike. I would anticipate a boolean type parameter for skip so when reading code logic, I need to translate skip to skipReason all the time, ending up with burden and pressure unnecessarily.

[katcharov]

This suppresses IntelliJ's parameter hinting. I have renamed it.

[NathanQingyangXu]

From my personal preference, I prefer the normal if block as following for it is the most natural, flexible and powerful:

if (isSharded() && serverVersionLessThan(5, 0)) {
     def.skipJira()
           .test(...)
           ...
}

Needless to say, the core code logic would be simplified to minimal for lots of details are simply for that when() usage. Is it necessary? I doubt. From coding perspective, it looks elegant and fluent, but if we take code maintaining cost into consideration, it might not be worth while. My personal preference, of course.

[katcharov]

The API is, I think, a way of abstracting away "if" - that is, skip and file are just if statements. For example, assumeFalse is another such API, which is only there to "abstract" away if statements. I do not think we should mix the approaches, without good reason. I originally had isSharded as an instance method, and this was redundant with the static method, which was a downside, but here, the lambda ensures that we can use static methods. I think this approach produces a cleaner API.

[NathanQingyangXu]

From my personal perspective, centralizing skips has pros and cons. The previous way has its pros. For one thing they are predicatable and easy to locate. Now it becomes hard to filter which ones is relevant for some specific unified testing (the test dir might be a clue but my first reaction is that might not be enough and I might start doubting the benefit of centralizing skips here).

The only undeniable benefit is to dedup across sync and reactive skips. As long as that could be done (there might be easy way like moving common stuff into some parent class?), honestly I think such coding effort might become a future maintaining burden.

[katcharov]

The primary benefit is that one is able to more easily see what test-specific custom logic the driver has, without needing to check multiple files, many of which may not actually have their own custom logic.

It also incrementally moves us to having a single file for tests, rather than multiple files.