Fix flaky options_bootstrapper_test.py
#9220
Conversation
| @@ -73,16 +74,16 @@ class DuplicateScopeError(Exception): | |||
| """More than one registration occurred for the same scope.""" | |||
|
|
|||
| @classmethod | |||
| def complete_scopes(cls, scope_infos: Iterable[ScopeInfo]) -> Set[ScopeInfo]: | |||
| def complete_scopes(cls, scope_infos: Iterable[ScopeInfo]) -> FrozenOrderedSet[ScopeInfo]: | |||
There was a problem hiding this comment.
Sometimes it's fine to return a Set, but generally FrozenOrderedSet is a much safer default. Especially with the engine, we want stable iteration order for caching.
| @@ -213,7 +214,7 @@ def get_bootstrap_options(self) -> Options: | |||
| return self.bootstrap_options | |||
|
|
|||
| @memoized_method | |||
| def _full_options(self, known_scope_infos: Tuple[ScopeInfo, ...]) -> Options: | |||
| def _full_options(self, known_scope_infos: FrozenOrderedSet[ScopeInfo]) -> Options: | |||
There was a problem hiding this comment.
This cannot be Iterable or Sequence because @memoized_method requires a hashable type so passing something like a list—then relying on the downstream implementation to sort—would fail.
| return self._full_options(tuple(set(known_scope_infos))) | ||
| return self._full_options( | ||
| FrozenOrderedSet(sorted(known_scope_infos, key=lambda si: si.scope)) | ||
| ) |
There was a problem hiding this comment.
This change is key. Previously, we were using set() to get the de-duplication and tuple to make it hashable. But, we weren't getting stable iteration order. Now we get all 3!
| all_options_under_scope = set(config.values.options(section)) - set( | ||
| config.values.defaults | ||
| ) |
There was a problem hiding this comment.
NB: we still use set here because they're inlined variables and only ever used for checking x in xs. There, it's simpler and faster to use the built-in set type.
| @@ -262,7 +265,7 @@ def verify_configs_against_options(self, options: Options) -> None: | |||
| all_options_under_scope = set(config.values.options(section)) - set( | |||
| config.values.defaults | |||
| ) | |||
| for option in all_options_under_scope: | |||
| for option in sorted(all_options_under_scope): | |||
There was a problem hiding this comment.
Not relevant to the flake, but this means our config validation error will now be deterministic!
…ptions-test # Conflicts: # src/python/pants/option/options_test.py
test_full_options_cachingwould frequently flake because we were using sets internally in theOptionsandOptionsBootstrappertypes. Sets have unstable ordering, so sometimes the test would pass and sometimes it would fail.We fix this by using our
OrderedSetandFrozenOrderedSettypes, with sorting before passing to those constructors.