[finishingmove]

We see the test checks three things:

  Verifies:
    - KogMaw deals less damage to non-lane minions
    - KogMaw deals percentile magic damage
    - KogMaw deals normal damage to lane minions

and the verify method has three assertions. In my opinion, this should be three tests, each with only one assertion.

[d0100]

They'd soon be drowning in individual tests. And it wouldn't necessarily fit to separate them, considering all those 3 have to be tested on each skill usage. One shot has to obey those 3 rules.

Making each an individual test would triple the test time, pollute the test codebase and not bring any advantage.

[finishingmove]

> Making each an individual test would triple the test time, pollute the test codebase and not bring any advantage.

Would it really? None of what you said here is true.

[quacker]

I think it is true. I mean, tripling the number of test cases when you already have 5500 of them is going to slow down your dev process in a significant way.

[chrislloyd]

The advantage in splitting up the assertions is that you get more visibility. If one test fails and not the other two that's extra information you can use to debug you wouldn't have if you have all 3 in one test.

[chii]

If that test fails, the dev will just run it locally with a debugger attached. The cost of extra time in the test for the normal passing case isn't worth it.

[quacker]

Right, but consider if all three test cases fail. Then devs (who didn't write those tests) need to determine what those three test cases are and if they're all the same issue or not, which takes time.

Ideally, if just one assert fails, it will produce an error message/logs with enough info to tell what went wrong. Having tests produce great error messages is way more valuable than arbitrary rules about assertion density.

[swanson]

I tend to agree, but sometimes in these kind of instrumented test environments, the setup cost of each test case might be very high so maybe the team found it reasonable to group all the assertions into a single test (especially given that the suite is already taking 1-2 hours to give feedback to developers).

[badloginagain]

It appears they're grouping by ability.

[finishingmove]

I am well aware of that and I agree on being pragmatic, but more often than not, the reason for multiple assertions per test is developer laziness and/or unawareness of the benefits of isolating failures by having just one assertion per test.

[0x0]

Seems to me you actually want all three assertions checked in the same run. Otherwise you could get three runs where only the first assertion was true in the first run; only the second assertion succeeding in the second run with the others failing; etc.

I bet the tests aren't pixel-by-pixel frame-by-frame reproducible (they are talking about a one week staging period for tests to prove themselves, so surely there's some non-deterministic wiggle room in remote controlling the full graphical client), so you really want to cover all assertions together for each run.

[finishingmove]

They should be tested independently, each with a completely reproducible setup that does only what is required for the assertion that will be made. I'm not sure what's hard to understand here... Are you trying to say that they need to be run together? Because it doesn't seem like that to me.

[dimino]

The code itself isn't super duper pythonic, which is not ideal, but definitely the right direction for a project like this to move.

It's just hard sometimes to set and follow standards when you're pushing to demonstrate value. Not impossible, but hard.