Commit 4be3f50
[Lens] Improve transform validation checking (elastic#243732)
## Summary
This PR is meant to improve the Lens transform validation framework.
Closes elastic#242663
Closes elastic#267582
Currently there are a few main problems:
- It's verbose when using over and over for the same chart
- It's split from the api-to-state validator
- It does not ever compare with the initial state when checking
state-to-api.
The first 2 are fixed but cleaning up the methods calls.
```ts
// The old way ❌
validateConverter(simpleMetricAttributes, metricStateSchema);
validateAPIConverter(simpleMetricApi, metricStateSchema);
// The new way ✅
validator.metric.fromState(simpleMetricAttributes);
validator.metric.fromApi(simpleMetricApi);
```
The 3rd issue, the biggest problem is that original configurations can
be completely wiped away and the test with still pass. The solution
before was to just test the transforms starting with the api then going
to state and back with `validateAPIConverter`. However, many of the
examples we have, like those from integrations, start from the SO.
The idea is to have a `normalizers` for each chart type that
[canonicalizes](https://grokipedia.com/page/Canonicalization) or
normalizes the original SO into a form that is comparable to the new SO
from the config builder output. This means:
- Replacing uuids with their respective human-readable ids
- Stripping or adding defaults
- Cleaning up properties. Mainly omitting `undefined` values instead of
explicitly setting the property (i.e. `"filter": undefined`).
Initially, this will be an **opt-in** parameter on the
`validator.fromState` method, then enforced always when all charts are
stabilized.
```ts
validator.metric.fromState(simpleMetricAttributes, true); // strict checking enabled
```
In a way this is a bit overkill but I think it is worth it to be able to
accurately test input to output 1 to 1, 🍎 to 🍏.
This normalization is does per chart with common logic shared between
them. The PR starts with the heatmap charts.
The framework allows both mutating the original SO and the transformed
SO (original SO -> api -> transformed SO) to make it easier to handle
per case changes. For example converting the original random ids to the
human readable ids is easier than vice versa. There is also an `ignore`
option to completely ignore comparing given fields, this should be the
last case as it's better to normalize to establish the baseline to
compare, rather than ignore it entirely. But in some cases it's just too
complex to do so.
We can compose these normalizers together for a given chart type to
include common normalizers but also to group normalizers but issue,
type, context, etc.. In the end joining them together with
`mergeNormalizers`.
```ts
const alignLegacyTypes: NormalizerConfig<HeatmapAttributes> = {
original: (attributes) => {
// mutate original attributes as needed.
attributes.state.visualization.legend.isVisible = isVisible ?? true;
return attributes;
},
transformed: (attributes) => {
// mutate transformed attributes as needed.
return attributes;
},
ignore: ['state.datasourceStates.formBased.layers.*.columns.*.params']
}
export const normalizeHeatmap = mergeNormalizers([alignLegacyTypes])
```
> [!IMPORTANT]
> The normalizers can mutate the `attributes` directly to make it easier
to tweak and this is just test code so I don't really care to get fancy.
Also the `attributes` should generally not be shared between tests so
there should be no interference.
These normalizers **only** apply to the `fromState` validator and only
apply when `strict` mode is set to `true` when comparing the original
and transformed states.
### Examples
The simplest case is we just compare what we put into the CB against the
SO that came out of the CB (SO -> API -> SO).
<details><summary>Simple Metric</summary>
<p>
```diff
- Expected - 18
+ Received + 14
@@ -1,61 +1,57 @@
Object {
"description": "",
"references": Array [
Object {
"id": "90943e30-9a47-11e8-b64d-95841ca0b247",
- "name": "indexpattern-datasource-layer-2821bd27-b805-4dea-a7d4-123c248e63b1",
+ "name": "indexpattern-datasource-layer-layer_0",
"type": "index-pattern",
},
],
"state": Object {
"adHocDataViews": Object {},
"datasourceStates": Object {
"formBased": Object {
"layers": Object {
- "2821bd27-b805-4dea-a7d4-123c248e63b1": Object {
+ "layer_0": Object {
"columnOrder": Array [
- "812a7944-731e-4967-8b84-1c8bba4ff04b",
+ "metric_formula_accessor_metric",
],
"columns": Object {
- "812a7944-731e-4967-8b84-1c8bba4ff04b": Object {
+ "metric_formula_accessor_metric": Object {
+ "customLabel": false,
"dataType": "number",
+ "filter": undefined,
"isBucketed": false,
- "label": "Count of records",
+ "label": "",
"operationType": "count",
"params": Object {
"emptyAsNull": true,
},
"sourceField": "___records___",
},
},
- "incompleteColumns": Object {},
+ "ignoreGlobalFilters": false,
"sampling": 1,
- },
- },
},
- "indexpattern": Object {
- "layers": Object {},
},
- "textBased": Object {
- "layers": Object {},
},
},
"filters": Array [],
"internalReferences": Array [],
"query": Object {
"language": "kuery",
"query": "",
},
"visualization": Object {
- "layerId": "2821bd27-b805-4dea-a7d4-123c248e63b1",
+ "collapseFn": undefined,
+ "layerId": "layer_0",
"layerType": "data",
- "metricAccessor": "812a7944-731e-4967-8b84-1c8bba4ff04b",
- "secondaryLabelPosition": "before",
- "secondaryTrend": Object {
- "type": "none",
- },
+ "metricAccessor": "metric_formula_accessor_metric",
+ "showBar": false,
+ "subtitle": "",
+ "valueFontMode": "default",
},
},
"title": "Lens Metric - By Ref",
"version": 1,
"visualizationType": "lnsMetric",
```
> Notice many things align but apart from the ids, only a few things are
mismatched.
</p>
</details>
If we replace all the uuids with their new respective human-readable
ids, the true issues become more apparent...
<details><summary>Simple Metric with ids replaced</summary>
<p>
```diff
- Expected - 12
+ Received + 8
@@ -16,46 +16,42 @@
"columnOrder": Array [
"metric_formula_accessor_metric",
],
"columns": Object {
"metric_formula_accessor_metric": Object {
+ "customLabel": false,
"dataType": "number",
+ "filter": undefined,
"isBucketed": false,
- "label": "Count of records",
+ "label": "",
"operationType": "count",
"params": Object {
"emptyAsNull": true,
},
"sourceField": "___records___",
},
},
- "incompleteColumns": Object {},
+ "ignoreGlobalFilters": false,
"sampling": 1,
},
- },
- },
- "indexpattern": Object {
- "layers": Object {},
},
- "textBased": Object {
- "layers": Object {},
},
},
"filters": Array [],
"internalReferences": Array [],
"query": Object {
"language": "kuery",
"query": "",
},
"visualization": Object {
+ "collapseFn": undefined,
"layerId": "layer_0",
"layerType": "data",
"metricAccessor": "metric_formula_accessor_metric",
- "secondaryLabelPosition": "before",
- "secondaryTrend": Object {
- "type": "none",
- },
+ "showBar": false,
+ "subtitle": "",
+ "valueFontMode": "default",
},
},
"title": "Lens Metric - By Ref",
"version": 1,
"visualizationType": "lnsMetric",
```
</p>
</details>
Going one step further and cleaning up both the common state and the
chart-specific visualization state, we see true issues, in this case
that the label is cleared and some default options are being applied. We
can ignore default options on a per-chart basis while still getting most
of the benefits of the comparison.
<details><summary>Simple Metric with ids replaced</summary>
<p>
```diff
- Expected - 12
+ Received + 8
@@ -16,46 +16,42 @@
"columnOrder": Array [
"metric_formula_accessor_metric",
],
"columns": Object {
"metric_formula_accessor_metric": Object {
+ "customLabel": false,
"dataType": "number",
"isBucketed": false,
- "label": "Count of records",
+ "label": "",
"operationType": "count",
"params": Object {
"emptyAsNull": true,
},
"sourceField": "___records___",
},
},
"ignoreGlobalFilters": false,
"sampling": 1,
},
},
},
"filters": Array [],
"internalReferences": Array [],
"query": Object {
"language": "kuery",
"query": "",
},
"visualization": Object {
"layerId": "layer_0",
"layerType": "data",
"metricAccessor": "metric_formula_accessor_metric",
- "secondaryLabelPosition": "before",
- "secondaryTrend": Object {
- "type": "none",
- },
+ "showBar": false,
+ "subtitle": "",
+ "valueFontMode": "default",
},
},
"title": "Lens Metric - By Ref",
"version": 1,
"visualizationType": "lnsMetric",
```
</p>
</details>
### Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
- [x] Review the [backport
guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)
and apply applicable `backport:*` labels.
---------
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: macroscopeapp[bot] <170038800+macroscopeapp[bot]@users.noreply.github.com>1 parent feb1371 commit 4be3f50
45 files changed
Lines changed: 2282 additions & 840 deletions
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
Lines changed: 10 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
56 | | - | |
| 56 | + | |
57 | 57 | | |
58 | 58 | | |
59 | 59 | | |
60 | | - | |
| 60 | + | |
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| |||
172 | 172 | | |
173 | 173 | | |
174 | 174 | | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
175 | 183 | | |
176 | 184 | | |
177 | 185 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| 33 | + | |
33 | 34 | | |
34 | 35 | | |
35 | 36 | | |
36 | 37 | | |
| 38 | + | |
37 | 39 | | |
38 | 40 | | |
39 | 41 | | |
| |||
Lines changed: 14 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
246 | 246 | | |
247 | 247 | | |
248 | 248 | | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
249 | 263 | | |
250 | 264 | | |
251 | 265 | | |
| |||
Lines changed: 28 additions & 31 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | | - | |
13 | 12 | | |
14 | 13 | | |
15 | 14 | | |
16 | | - | |
| 15 | + | |
17 | 16 | | |
18 | 17 | | |
19 | 18 | | |
| |||
48 | 47 | | |
49 | 48 | | |
50 | 49 | | |
51 | | - | |
| 50 | + | |
52 | 51 | | |
53 | | - | |
| 52 | + | |
54 | 53 | | |
55 | 54 | | |
56 | 55 | | |
57 | | - | |
| 56 | + | |
58 | 57 | | |
59 | 58 | | |
60 | 59 | | |
61 | | - | |
| 60 | + | |
62 | 61 | | |
63 | 62 | | |
64 | 63 | | |
65 | | - | |
| 64 | + | |
66 | 65 | | |
67 | 66 | | |
68 | 67 | | |
69 | | - | |
| 68 | + | |
70 | 69 | | |
71 | 70 | | |
72 | 71 | | |
73 | | - | |
| 72 | + | |
74 | 73 | | |
75 | 74 | | |
76 | 75 | | |
77 | | - | |
| 76 | + | |
78 | 77 | | |
79 | 78 | | |
80 | 79 | | |
81 | | - | |
| 80 | + | |
82 | 81 | | |
83 | 82 | | |
84 | 83 | | |
85 | | - | |
| 84 | + | |
86 | 85 | | |
87 | 86 | | |
88 | 87 | | |
89 | | - | |
| 88 | + | |
90 | 89 | | |
91 | 90 | | |
92 | 91 | | |
93 | | - | |
94 | | - | |
95 | | - | |
96 | | - | |
| 92 | + | |
97 | 93 | | |
98 | 94 | | |
99 | 95 | | |
100 | | - | |
| 96 | + | |
101 | 97 | | |
102 | 98 | | |
103 | 99 | | |
104 | | - | |
| 100 | + | |
105 | 101 | | |
106 | 102 | | |
107 | | - | |
| 103 | + | |
| 104 | + | |
108 | 105 | | |
109 | | - | |
| 106 | + | |
110 | 107 | | |
111 | 108 | | |
112 | 109 | | |
113 | | - | |
| 110 | + | |
114 | 111 | | |
115 | 112 | | |
116 | 113 | | |
117 | | - | |
| 114 | + | |
118 | 115 | | |
119 | 116 | | |
120 | 117 | | |
121 | | - | |
| 118 | + | |
122 | 119 | | |
123 | 120 | | |
124 | 121 | | |
125 | | - | |
| 122 | + | |
126 | 123 | | |
127 | 124 | | |
128 | 125 | | |
129 | | - | |
| 126 | + | |
130 | 127 | | |
131 | 128 | | |
132 | 129 | | |
133 | | - | |
| 130 | + | |
134 | 131 | | |
135 | 132 | | |
136 | 133 | | |
137 | | - | |
| 134 | + | |
138 | 135 | | |
139 | 136 | | |
140 | 137 | | |
141 | | - | |
| 138 | + | |
142 | 139 | | |
143 | 140 | | |
144 | 141 | | |
145 | | - | |
| 142 | + | |
146 | 143 | | |
147 | 144 | | |
148 | 145 | | |
149 | | - | |
| 146 | + | |
150 | 147 | | |
151 | 148 | | |
152 | 149 | | |
| |||
Lines changed: 16 additions & 16 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | | - | |
| 12 | + | |
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
17 | 16 | | |
18 | 17 | | |
19 | 18 | | |
| |||
31 | 30 | | |
32 | 31 | | |
33 | 32 | | |
34 | | - | |
| 33 | + | |
35 | 34 | | |
36 | | - | |
| 35 | + | |
37 | 36 | | |
38 | 37 | | |
39 | 38 | | |
40 | | - | |
| 39 | + | |
41 | 40 | | |
42 | 41 | | |
43 | | - | |
44 | | - | |
| 42 | + | |
| 43 | + | |
45 | 44 | | |
46 | 45 | | |
47 | 46 | | |
48 | | - | |
| 47 | + | |
49 | 48 | | |
50 | 49 | | |
51 | 50 | | |
52 | | - | |
| 51 | + | |
53 | 52 | | |
54 | 53 | | |
55 | | - | |
| 54 | + | |
| 55 | + | |
56 | 56 | | |
57 | | - | |
| 57 | + | |
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
61 | | - | |
| 61 | + | |
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
65 | | - | |
| 65 | + | |
66 | 66 | | |
67 | 67 | | |
68 | 68 | | |
69 | | - | |
| 69 | + | |
70 | 70 | | |
71 | 71 | | |
72 | 72 | | |
73 | | - | |
| 73 | + | |
74 | 74 | | |
75 | 75 | | |
76 | 76 | | |
77 | | - | |
| 77 | + | |
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| |||
0 commit comments