4 ms·

We see the test checks three things: Verifies: - KogMaw deals less damage to non-lane minions - KogMaw deals percentile magic damage - KogMaw dea

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.

swanson 10y ago
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 10y ago
  It appears they're grouping by ability.
- finishingmove 10y ago
  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 10y ago
    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 10y ago
      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.
d0100 10y ago
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.
- chrislloyd 10y ago
  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 10y ago
    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 10y ago
    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.
- finishingmove 10y ago
  > 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 10y ago
    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.
dimino 10y ago
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.

swanson 10y ago

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 10y ago
It appears they're grouping by ability.
finishingmove 10y ago
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 10y ago
  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 10y ago
    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.

d0100 10y ago

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.

chrislloyd 10y ago
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 10y ago
  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 10y ago
  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.
finishingmove 10y ago
> 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 10y ago
  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.

dimino 10y ago

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.