Refactor RunList, add test suite #127
Conversation
yebrahim
force-pushed
the
yebrahim/run-list-tests
branch
from
c90ddb0
to
48f3386
Compare
yebrahim
changed the title
| expect(noMetricValueTree).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it('renders a empty metric container when a metric has value of zero', () => { |
There was a problem hiding this comment.
this is empty? so the same as the above test where there's no data at all?
There was a problem hiding this comment.
Yes. Currently the metric is validated to either be a percentage metric, in which case a value of zero will show something, or a raw metric, in which case maximum and minimum values are required in order for it to show something. This tests that for a raw metric (this is the default format), if no max/min values are specified, nothing shows up.
We need to make this more resilient, but it's the current code that I'm testing.
There was a problem hiding this comment.
Added more tests around this.
| } | ||
|
|
||
| /** | ||
| * For each DisplayRun, get its ApiRun and retrieve that ApiRun's Pipeline ID if it has one, then | ||
| * use that Pipeline ID to fetch its associated Pipeline and attach that Pipeline's name to the | ||
| * DisplayRun. If the ApiRun has no Pipeline ID, then the corresponding DisplayRun will show '-'. | ||
| */ | ||
| private async _getAndSetPipelineNames(displayRuns: DisplayRun[]): Promise<DisplayRun[]> { | ||
| return await Promise.all( | ||
| private _getAndSetPipelineNames(displayRuns: DisplayRun[]): Promise<DisplayRun[]> { |
There was a problem hiding this comment.
nit: why not leave the await and async?
There was a problem hiding this comment.
Just because it's more code. With function signature enforced, I don't think adding the await adds more safety over returning the promise. wdyt?
yebrahim
force-pushed
the
yebrahim/run-list-tests
branch
from
48f3386
to
0063754
Compare
Pull Request Test Coverage Report for Build 354
💛 - Coveralls |
Pull Request Test Coverage Report for Build 320
💛 - Coveralls |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rileyjbauer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rileyjbauer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
yebrahim
force-pushed
the
yebrahim/run-list-tests
branch
from
746dc05
to
e0355de
Compare
|
/lgtm |
|
/lgtm |
|
/test presubmit-e2e-test |
* fix doc structure * fix typo
RunListto have one code path for loading runs, which branches only to get the list of run IDs.RunList.extractMetricMetadataintoRunUtilsfor easier testing (not done yet).CustomTableinstances, since this is no longer maintained by the parent component.This change is