Skip to content

Commit a370995

Browse files
committed
[REF] tests: remove chart snapshot tests
There were a dozen chart snapshot tests in `chart_plugin.test.ts`. Some tests were relying exclusively on snapshots. The main issue is that snapshots don't actually test a feature. Reading the test provides no insight on what is actually tested, and after years of snapshot updates, who's to say that the relevant part of the snapshot hasn't changed ? Or that the 400 lines of snapshot for a new test were ever fully reviewed ? Snapshots shouldn't be the main way a feature is tested. This commit removes most snapshot tests, and replaces them with more explicit `expect` matchers. Some snapshots were kept because a snapshot test is still useful as an alarm bell for the coder/reviewer (eg. "why has this pie bar fix changed snapshots of bar charts ?"). But having a dozen snapshot is pointless, making it more likely that the changes are never reviewed. closes #7313 Task: 5160031 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
1 parent 49d5142 commit a370995

File tree

7 files changed

+175
-3010
lines changed

7 files changed

+175
-3010
lines changed

packages/o-spreadsheet-engine/src/types/chart/bar_chart.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ export interface BarChartDefinition extends CommonChartDefinition {
1010
}
1111

1212
export type BarChartRuntime = {
13-
chartJsConfig: ChartConfiguration;
14-
masterChartConfig?: ChartConfiguration;
13+
chartJsConfig: ChartConfiguration<"bar" | "line">;
14+
masterChartConfig?: ChartConfiguration<"bar">;
1515
background: Color;
1616
};

packages/o-spreadsheet-engine/src/types/chart/line_chart.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ export interface LineChartDefinition extends CommonChartDefinition {
1414
}
1515

1616
export type LineChartRuntime = {
17-
chartJsConfig: ChartConfiguration;
18-
masterChartConfig?: ChartConfiguration;
17+
chartJsConfig: ChartConfiguration<"line">;
18+
masterChartConfig?: ChartConfiguration<"line">;
1919
background: Color;
2020
};

src/helpers/figures/charts/bar_chart.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ export function createBarChartRuntime(chart: BarChart, getters: Getters): BarCha
224224
const definition = chart.getDefinition();
225225
const chartData = getBarChartData(definition, chart.dataSets, chart.labelRange, getters);
226226

227-
const config: ChartConfiguration = {
227+
const config: ChartConfiguration<"bar" | "line"> = {
228228
type: "bar",
229229
data: {
230230
labels: chartData.labels,

src/helpers/figures/charts/runtime/chartjs_scales.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@ type GeoChartScales = DeepPartial<ScaleChartOptions<"choropleth">["scales"]>;
5050
export function getBarChartScales(
5151
definition: GenericDefinition<BarChartDefinition>,
5252
args: ChartRuntimeGenerationArgs
53-
): ChartScales {
54-
let scales: ChartScales = {};
53+
): DeepPartial<ScaleChartOptions<"line" | "bar">["scales"]> {
54+
let scales: DeepPartial<ScaleChartOptions<"line" | "bar">["scales"]> = {};
5555
const { trendDataSetsValues: trendDatasets, locale, axisFormats } = args;
5656
const options = { stacked: definition.stacked, locale: locale };
5757
if (definition.horizontal) {
@@ -90,12 +90,12 @@ export function getBarChartScales(
9090
export function getLineChartScales(
9191
definition: GenericDefinition<LineChartDefinition>,
9292
args: ChartRuntimeGenerationArgs
93-
): ChartScales {
93+
): DeepPartial<ScaleChartOptions<"line">["scales"]> {
9494
const { locale, axisType, trendDataSetsValues: trendDatasets, labels, axisFormats } = args;
9595
const labelFormat = axisFormats?.x;
9696
const stacked = definition.stacked;
9797

98-
let scales: ChartScales = {
98+
let scales: DeepPartial<ScaleChartOptions<"line">["scales"]> = {
9999
x: getChartAxis(definition, "bottom", "labels", { locale }),
100100
y: getChartAxis(definition, "left", "values", { locale, stacked, format: axisFormats?.y }),
101101
y1: getChartAxis(definition, "right", "values", { locale, stacked, format: axisFormats?.y1 }),

src/helpers/figures/charts/scatter_chart.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ export function createScatterChartRuntime(
234234
const definition = chart.getDefinition();
235235
const chartData = getLineChartData(definition, chart.dataSets, chart.labelRange, getters);
236236

237-
const config: ChartConfiguration = {
237+
const config: ChartConfiguration<"line"> = {
238238
// use chartJS line chart and disable the lines instead of chartJS scatter chart. This is because the scatter chart
239239
// have less options than the line chart (it only works with linear labels)
240240
type: "line",

0 commit comments

Comments
 (0)