minsev: Refactor unit tests #5869
Conversation
pellared
added
the
Skip Changelog
| wrapped := &processor{ReturnErr: assert.AnError} | ||
| p := NewLogProcessor(wrapped, api.SeverityTrace1) | ||
| ctx := context.Background() | ||
| r := &log.Record{} |
There was a problem hiding this comment.
Isn't this allocating every iteration instead of once?
There was a problem hiding this comment.
Is it a problem? It is just a unit test (not even integration test). It is not a benchmark and it does not make the test execution noticeable longer.
There was a problem hiding this comment.
Is it a problem? It is just a unit test (not even integration test). It is not a benchmark and it does not make the test execution noticeable longer.
How does this line of reasoning not apply to changes submitted in this PR in general? This seems to be a subjective change with functionally equivalent code being produced. Am I missing why this shouldn't be evaluated on these merits?
There was a problem hiding this comment.
It reduces the cyclomatic complexity, the number lines of code, API of test double. I find this simpler, more readable and maintainable than requiring future developers to think when the processor needs to be reset. I do not find that simplifying test code is subjective.
There was a problem hiding this comment.
It reduces the cyclomatic complexity, the number lines of code, API of test double. I find this simpler, more readable and maintainable than requiring future developers to think when the processor needs to be reset. I do not find that simplifying test code is subjective.
Right, and I am pointing out that your version, that you point out will be copied, has memory issues that should not be introduced.
I'm still not following how your subjective feelings of simplicity are more valid to the criticism of "Is it a problem? It is just a unit test (not even integration test). It is not a benchmark and it does not make the test execution noticeable longer."
Are these changes just subjective feel? If so I think performance evaluations like the one provided in this comment thread need to be evaluated.
There was a problem hiding this comment.
I removed Reset method (API) from processor type ( which is a test double)
There was a problem hiding this comment.
You mean benchmark a unit test?
I'm not sure you need a benchmark to see the inefficient memory design of the test.
There was a problem hiding this comment.
Just to have some quote https://wiki.c2.com/?StructuredProgrammingWithGoToStatements
Programmers waste enormous amounts of time thinking about, or worrying about, the speed of noncritical parts of their programs, and these attempts at efficiency actually have a strong negative impact when debugging and maintenance are considered. We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil.
I just think we do need to care about performance in this scenario.
Yet we should not pass up our opportunities in that critical 3%.
I do not think this is this 3% 😉
There was a problem hiding this comment.
Sure, but if you replace "speed" with "code quality" with what you quoted, the same applies to the changes being presented in this PR.
Like I said above, I'm not sure any of this discussion if really justified given the functional test coverage does not change with this PR.
There was a problem hiding this comment.
given the functional test coverage does not change with this PR
From https://en.wikipedia.org/wiki/Code_refactoring:
Refactoring is intended to improve the design, structure, and/or implementation of the software (its non-functional attributes), while preserving its functionality. Potential advantages of refactoring may include improved code readability and reduced complexity
Sure, but if you replace "speed" with "code quality" with what you quoted, the same applies to the changes being presented in this PR.
Feel free to close the PR if you are against the change. As the codeowner I think you the right to do it.
I created the PR as part of my work on #5861.
| r.SetSeverity(sev) | ||
| assert.NoError(t, p.OnEmit(ctx, *r), assert.AnError, sev.String()) | ||
|
|
||
| if !assert.Lenf(t, wrapped.OnEmitCalls, 0, "Record with severity %s passed-through", sev) { |
There was a problem hiding this comment.
This if increases the https://en.m.wikipedia.org/wiki/Cyclomatic_complexity
There was a problem hiding this comment.
I think I gotcha. It seems a bit like the cyclomatic complexity is being over applied here, but if that is truly the desire, there is no need for the if statement.
assert.Lenf(t, wrapped.OnEmitCalls, 0, "Record with severity %s passed-through", sev)
wrapped.Reset()That would avoid if statement you are concerned about without the added memory inefficiencies. There would still be computation efficiencies as the function call will be a no-op in the existing true conditional case.
I'm still not sold that this metric is really serving to better the code with type of change though.
|
Minsev is so small (and well designed) that we do not need to gold plate the tests. Closing. |
Refactoring that I suggested here: #5817 (comment) (it was not a blocker).
I think it makes the tests simpler. The reader and writer does not need to think when the wrapped processor needs to be reset as we recreate a new one for each test case.