Sort studies by name and add basic study intersection check. #1023
Conversation
goodov
changed the title
| @@ -213,6 +281,7 @@ def main(): | |||
|
|
|||
| print("Load", args.seed_path.name) | |||
| seed_data = json.load(args.seed_path) | |||
| seed_data['studies'].sort(key=lambda study: study['name']) | |||
There was a problem hiding this comment.
I suppose python sort is stable?
Let's add a comment we can't reorder the study with the same name.
There was a problem hiding this comment.
why can't we reorder the studies with the same name if they do not overlap by filters?
There was a problem hiding this comment.
If they don't overlap there is no problem.
If I'm not mistaken overlapped studies with the same name is used in Yandex.
There was a problem hiding this comment.
If they don't overlap there is no problem. If I'm not mistaken overlapped studies with the same name is used in Yandex.
We should not allow overlapped studies, added checks ensures we don't.
seed/serialize.py
Outdated
| version_list = [] | ||
| for part in parts: | ||
| if part == '*': | ||
| break |
There was a problem hiding this comment.
Hm, I'm not 100% sure about this part.
* means different for min/max version.
as for Python is alway [1,2] > [1]
There was a problem hiding this comment.
Hm, I'm not 100% sure about this part.
*means different for min/max version. as for Python is alway[1,2] > [1]
yes, you're right. This comparison is tricky. Fixed and add tests from the C++ version.
| version1 = version_to_int_array(test_case[0]) | ||
| version2 = version_to_int_array(test_case[1]) | ||
| assert compare_versions(version1, version2) == test_case[2] | ||
|
|
There was a problem hiding this comment.
this is pretty dirty (having tests and logic in the same file), but we should remove this file entirely in the near future.
| test_version_comparison() | ||
| for studies in feature_names_to_studies.values(): | ||
| for i, study1 in enumerate(studies): | ||
| study1_platform = get_study_platforms(study1) |
There was a problem hiding this comment.
Could study_platform or study_channel be empty?
There was a problem hiding this comment.
Could
study_platformorstudy_channelbe empty?
yep. improved the intersection check to cover this.
…is is not allowed).
* Sort studies by name and add basic study intersection validation. * Fix wildcard version comparison, add tests. * Make sure to properly cover empty channel/platform lists (although this is not allowed).
Sort studies by name before generating seed.
This is a required change to migrate to per-file study structure, because per-file structure implies study enumeration in alphabetical order (as files lay on the file system).
This change should be a no-op as long as we don't have studies intersection. This PR adds the required validation to ensure we do not have this kind of intersection, which means if the seed is validated, then it doesn't depend on the initial order of studies in
seed.jsonand it's safe to reorder studies.Related brave/brave-browser#33654