Fix get health #1032
Fix get health #1032
Conversation
arbulu89
requested review from
dottorblaster,
fabriziosestito,
rtorrero,
nelsonkopliku,
CDimonaco,
EMaksy and
jamie-suse
dottorblaster
force-pushed
the
fix-get-health
branch
from
80c8a11
to
acdfe32
Compare
dottorblaster
force-pushed
the
fix-get-health
branch
from
34d0405
to
660af78
Compare
dottorblaster
force-pushed
the
fix-get-health
branch
2 times, most recently
from
5138f14
to
dd40854
Compare
dottorblaster
force-pushed
the
fix-get-health
branch
from
dd40854
to
b57a126
Compare
dottorblaster
force-pushed
the
fix-get-health
branch
from
3af0432
to
11c45a6
Compare
| ); | ||
|
|
||
| export const getChecks = (checkResults) => | ||
| checkResults.map(({ check_id }) => check_id); | ||
|
|
||
| export const getHealth = (checkResults, checkID, agentID) => { | ||
| export const getCheckHealthByAgent = (checkResults, checkID, agentID) => { |
There was a problem hiding this comment.
would it make more sense to call this getCheckResultHealthByAgent ?
| } | ||
|
|
||
| // expectation evaluation error , malformed expression most probably | ||
| const evaluationErrors = agentCheckResult?.expectation_evaluations.filter( |
There was a problem hiding this comment.
I personally like adding a Count suffix to variables that are just counting something, in this case e.g.: evaluationErrorCount as it makes operations with the variable more obvious (e.g. the + below is immediately understood as a sum and not a concatenation). Wdyt?
| }; | ||
|
|
||
| const addExpectation = (checkResult, name, expec, result) => { | ||
| const { type } = expec; |
There was a problem hiding this comment.
This should be expectation to me, you're right
| return addExpectation(checkResult, name, expec, false); | ||
| }; | ||
|
|
||
| export const addExpectationWithError = (checkResult) => { |
There was a problem hiding this comment.
maybe addErroredExpectation sound better and matches the same wording as the other functions?
There was a problem hiding this comment.
@dottorblaster It looks good.
Have a look only in one of the factory functions that I think you updated accidentaly.
Besides that, a small change in the test to make it more logical.
Green light, otherwise.
I cannot approve though, as I opened the PR XD
| ); | ||
|
|
||
| export const executionFactFactory = Factory.define(() => ({ | ||
| check_id: faker.datatype.uuid(), | ||
| name: faker.animal.cat(), | ||
| value: faker.datatype.number(), | ||
| result: resultEnum(), |
There was a problem hiding this comment.
This change looks wrong. This is an execution fact, which is composed by check_id, name and value, as it was before
| import { renderWithRouter } from '@lib/test-utils'; | ||
| import { | ||
| addCriticalExpectation, |
There was a problem hiding this comment.
Is there any reason not to put the executions under the factories import. All of them are exported in the index file.
@lib/test-utils/factories
| @@ -87,7 +108,8 @@ describe('ExecutionResults', () => { | |||
| const { | |||
| clusterID, | |||
| hostnames, | |||
| checkID, | |||
| checkID1, | |||
There was a problem hiding this comment.
Should we return maybe checks? As we do with the hostnames
| } from '@lib/test-utils/factories/executions'; | ||
| import { catalogFactory, hostnameFactory } from '@lib/test-utils/factories'; | ||
|
|
||
| import ExecutionResults from './ExecutionResults'; | ||
|
|
||
| const prepareStateData = (checkExecutionStatus) => { | ||
| const hostnames = hostnameFactory.buildList(2); | ||
| const hostnames = hostnameFactory.buildList(4); |
There was a problem hiding this comment.
Leftover from an experiment LOL
| }); | ||
|
|
||
| const lastExecution = { | ||
| executionLoading: false, | ||
| executionData: { | ||
| status: checkExecutionStatus, | ||
| targets, | ||
| check_results: [checkResult], | ||
| check_results: [checkResult1, checkResult2], |
There was a problem hiding this comment.
@dottorblaster You fortot to change this executionData: executionResult
Improve the old
getHealth(nowgetCheckHealthByAgent) to consider all the possible scenarios: agentCheckError, expect_same, fact gathering error, etcI have added more factory improvements in order to make the check results mocks creation easy.
PD: I have fixed some other functions like
getHostsby this will be removed once we get the other PR using thetargetsto create the results table