Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(fr): add base fraggle rock snapshot runner #11748
core(fr): add base fraggle rock snapshot runner #11748
Changes from 1 commit
14fdb68
a79b8a7
7525e87
a7a860d
13ba78c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one day we should split this off into a
smoke-static-server-entry.js
or something ..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all FR gatherers going to support snapshot mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not all, see full compat table for which ones, but the next step after #11759 is the TODO that's here about using gatherer.meta to annotate whether a gatherer supports it or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name
isFRGatherer
makes it sound like this will detect FR gatherers, but it's used to detect snapshot support on L60. WDYT of renaming this to something likeisFRSnapshotGatherer
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, that's exactly what it's supposed to be doing :) purely a type guard on whether a gatherer instance supports the FR gatherer properties.
I consider this function not detecting snapshot support at all which is why the TODO is there. Any suggestion for alternative wording there to make it clearer? Snapshot detection support is 2-3 PRs away from being fixed and won't affect any results in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this issue has more to do with me misunderstanding the comment on L59 than the comment's wording. It's fine to leave as is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you're already thinking about how these types are mostly useless (sometimes worse than useless) in a world where we can't usually assume they're all actually there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important to note this section was just copied from our existing gathering so it's not a degradation :)
I have not invested significant time on ideating how to improve the types here, no. I don't really consider the pre-existing temporary lie of
LH.Artifacts
at the runner level to be high on the list of type migrations that need to be done to get FR off the ground. Do you think this type flaw we have today is much more important in the FR world to warrant addressing soon?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this ends up expanding to a bunch of new smoke tests, are you thinking about moving them somewhere else/called somehow else instead of in the midst of all the (mostly) unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
called somehow else, yes definitely 👍
moved somewhere else, no wasn't planning on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does changing this do?
typeof Gatherer
already is aClassOf<Gatherer.GathererInstance>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #11748 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is working around an old bug and should be able to just be
instance: Gatherer
, fwiwThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. The js class is the base class and the interface. Duplicating it here doesn't seem to do more with the type and adds to maintenance/distancing types from jsdocs, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decoupling the implementation and the interface is the goal here to distinguish the new from the old and differentiate which gatherers support which models of execution in a relatively trivial and lightweight way.
Is there a counter proposal in there I'm missing for how incremental transition for gatherers will be supported? I'm happy to move the docs to the interface if that's what you're asking.