Skip to content

Commit de78b89

Browse files
authored
Revamp backward compat tests (#5686)
1 parent a4a3d2a commit de78b89

File tree

3 files changed

+278
-103
lines changed

3 files changed

+278
-103
lines changed
+57-55
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,37 @@
11
# Backward compatibility in Quickwit.
22

33
If you are reading this, chances are you want to make a change to one of the resource
4-
of Quickwit's meta/config:
4+
of Quickwit's meta/config.
55

6-
User edited:
6+
There are basically 3 types of configuration:
7+
8+
Edited by the user and read back from file on startup:
79
- QuickwitConfig
810

9-
User edited and stored in metastore:
11+
Edited by the user then stored in the metastore:
1012
- IndexConfig
1113
- SourceConfig
14+
- VersionedIndexTemplate
1215

13-
Stored in metastore only:
16+
Assembled by Quickwit then stored in the metastore:
1417
- IndexMetadata
15-
- FileBackedIndex
16-
- SplitMetadata.
18+
- SplitMetadata
19+
- FileBackedIndex (file backed metastore only)
20+
- Manifest (file backed metastore only)
21+
22+
Quickwit currently manages the backward compatibility of all of these resources except the `QuickwitConfig`.
1723

18-
Quickwit currently manages backward compatibility of all of these resources but QuickwitConfig.
19-
This document describes how to handle a change, and how to make test such a change,
20-
and spot eventual regression.
24+
This document describes how to handle a change, and how to make test such a change, and spot eventual regression.
2125

22-
## How do I update `{IndexMetadata, SplitMetadata, FileBackedIndex, SourceConfig, IndexConfig}`?
26+
## How do I update `{IndexMetadata, SplitMetadata, FileBackedIndex, SourceConfig, IndexConfig, Manifest}`?
2327

24-
There are two types of upgrades.
28+
There are two types of upgrades:
29+
- naturally backward compatible change
30+
- change requiring a new version
2531

2632
### Naturally backward compatible change
2733

28-
Serde offers some attributes to make backward compatible change to our model.
34+
Serde offers some attributes to make backward compatible changes to our model.
2935
For instance, it is possible to add a new field to a struct and slap
3036
a `serde(default)` attribute to it in order to handle older serialized version of the
3137
struct.
@@ -37,88 +43,84 @@ it is recommended to not use it.
3743
It is also possible to rename a field in a backward compatible manner
3844
by using the `#[serde(alias)]`.
3945

40-
4146
For this type of change it is not required to update the serialization version.
4247

43-
We have a some mechanism (see backward compatibility test project below) to spot
44-
non-regression.
45-
46-
When introducing such a change:
48+
Nevertheless, the regression tests will spot these changes. When that happens:
4749
- modify your model with the help of the attributes above.
4850
- modify the example for the model by editing its `TestableForRegression` trait implementation.
49-
- commit the 2 files that were updated by build.rs
50-
- eyeball the diff on the `.expected.json` that failed, and send it with your PR.
51+
- run the backward compatibility tests (see below)
52+
- check the diff between the `xxx.modified.json` files created and the matching `xxx.json` files.
53+
If the changes are acceptable, replace the content of the `xxx.json` files and commit them.
54+
55+
Be particularly careful to changes on files corresponding to the most recent version. If the
56+
changes are not compatible, create a new configuration version.
5157

5258
### Change requiring a new version
5359

54-
For heavier changes requiring a new version, you will have to add to increment the configuration
55-
version. Please make sure that all of these resources share the same version number.
60+
For changes requiring a new version, you will have to increment the configuration
61+
version. You need to make sure that all of these resources share the same version number.
5662

5763
- update the resource struct you want to change.
5864
- create a new item in the `VersionedXXXX` struct. It is usually located in a serialize.rs file
59-
- `Serialize` is not needed for the previous serialized version. We just need `Deserialize`. We can remove the `Serialize` impl from the derive statement, and mark it a `skip_serializing` as follows.
65+
- `Serialize` is not needed for the previous serialized version. We just need `Deserialize`. We can
66+
remove the `Serialize` impl from the derive statement, and mark it a `skip_serializing` as follows.
6067

6168
e.g.
6269
```
6370
#[serde(tag = "version")]
6471
pub(crate) enum VersionedXXXXXX {
6572
#[serde(rename = "0")]
66-
V0(#[serde(skip_serializing)] XXXXV0),
73+
V0(#[serde(skip_serializing)] XXXX_V0),
6774
#[serde(rename = "1")]
68-
V1(XXXXXX1),
75+
V1(XXXX_V1),
6976
}
7077
```
71-
- Complete the conversion from VersionedXXXX to
72-
- make sure the conversion `From<XXXX> for VersionedXXXX` creates the new item.
73-
- run unit tests
74-
- commit the 2 files that were autogenerated by build.rs
75-
- eyeball and commit the autoupdated `expected.json` files
76-
- Possibly update the generation of the default XXXX instance used for regression. It is in the function `sample_for_regression_tests`.
78+
- complete the conversion `From<VersionedXXXX> for XXXX` and `From<XXXX> for VersionedXXXX`
79+
- run the backward compatibility tests (see below)
80+
- for older versions, check the diff between the `xxx.expected.modified.json` files created and the matching `xxx.expected.json` files.
81+
If the changes are acceptable, replace the content of the `xxx.expected.json` files and commit them.
82+
- check the the `yyyy.json` that was created for the new version and commit it along with the `yyyy.expected.json` file (identical).
83+
- possibly update the generation of the default XXXX instance used for regression. It is in the function `TestableForRegression::sample_for_regression`.
7784

7885

79-
## Backward compatibility test project.
86+
## Backward compatibility tests
8087

81-
This is just a project used to test backward compatibility of Quickwit.
82-
Right now, `SplitMetadata`, `IndexMetadata`, and `FileBackedIndex` are tested.
88+
These tests are used to ensure the backward compatibility of Quickwit.
89+
Right now, `SplitMetadata`, `IndexMetadata`, `Manifest` and `FileBackedIndex` are tested.
8390

8491
We want to be able to read all past versions of these files, but only write the most recent format.
85-
The tests consist of pairs of JSON files.
86-
XXXX.json and XXXX.expected.json
87-
88-
XXXX.json consists of a JSON file in an old format.
89-
XXXX.expected.json consists of the expected result of
90-
serialize_new_version(deserialize(XXXX.json)).
9192

92-
## Change of format autodetection
93+
The tests consist of pairs of JSON files, `XXXX.json` and `XXXX.expected.json`:
94+
- `XXXX.json` is the first serialized value of a new version.
95+
- `XXXX.expected.json` is the result of `serialize_new_version(deserialize(XXXX.json))`.
9396

94-
Two things need to happen upon a change of format.
97+
Format changes are automatically detected. There are two possible situations when a format changes.
9598

96-
### Updating expected.json
99+
#### Updating expected.json
97100

98-
Of course to make all of this work, we need to keep `*.expected.json` files up-to-date
99-
with the changes of format.
101+
We need to keep `*.expected.json` files up-to-date with the format changes.
100102

101103
This is done in a semi-automatic fashion.
102104

103-
On a first pass, `deserialize(original_json) == deserialize(expectation_json)`.
104-
On a second pass, the tests are checking that `expectation_json = serialize(deserialize(expectation_json))`.
105+
Checks are performed in two steps:
106+
- first pass, `deserialize(original_json) == deserialize(expectation_json)`
107+
- second pass, `expectation_json = serialize(deserialize(expectation_json))`
105108

106109
When changing the json format, it is expected to see this test fail.
107110
The unit test then updates automatically the `expected.json`. The developer just has to
108-
check the diff of the result (in particular no information should be lost) and commit the updated expected.json files.
111+
check the diff of the result (in particular no information should be lost) and commit the
112+
updated expected.json files.
109113

110114
Adding this update operation within the unit test is a tad unexpected, but it has the merit of
111115
integrating well with CI. If a developer forgets to update the expected.json file,
112116
the CI will catch it.
113117

114-
### Adding a new test case.
115-
116-
If the serialization format changes, the unit test will also automatically add
117-
a new unit test generated from a sample tested object.
118-
Concretely, it will just write two files `XXXX.json` and `XXXX.expected.json`.
118+
#### Adding a new test case.
119119

120-
The two files will be identical. This is expected as this is a unit test for the
121-
most recent version.
120+
If the serialization format changes, an new version should be created and the unit test will
121+
automatically add a new unit test generated from the sample tested objects.
122+
Concretely, it will just write two files `XXXX.json` and `XXXX.expected.json` for each model.
122123

123-
The unit test will start making sense in future updates thanks to the update phase
124+
The two files will be identical. This is expected as this is a unit test for the most recent
125+
version. The unit test will start making sense in future updates thanks to the update phase
124126
described in the previous section.

0 commit comments

Comments
 (0)