Extract and genericize Graph for easier testing #6010
Conversation
stuhood
force-pushed
the
stuhood/extract-graph
branch
from
b1b4a88 to
775e8cb
Compare
stuhood
force-pushed
the
stuhood/extract-graph
branch
from
868cb58 to
9358577
Compare
src/rust/engine/graph/src/node.rs
Outdated
| fn format(&self) -> String; | ||
|
|
||
| /// | ||
| /// If this Node represents an FS operation, returns its input Path. |
There was a problem hiding this comment.
What kind of path? Absolute? Relative to build root?
There was a problem hiding this comment.
You trolled me into making this more generic.
| /// | ||
| pub trait NodeTracer<N: Node> { | ||
| /// | ||
| /// Returns true if the given Node Result represents the "bottom" of a trace. |
There was a problem hiding this comment.
What does bottom mean? A leaf node? The most recently executed stack frame? The opposite of one of the above?
src/rust/engine/src/scheduler.rs
Outdated
| fn create(&self, entry_id: EntryId) -> Context { | ||
| Context::new(entry_id, self.clone()) | ||
| #[derive(Clone)] | ||
| struct RootContext { |
There was a problem hiding this comment.
What value is this providing over just using an Arc<Core>?
There was a problem hiding this comment.
Is this because the NodeContext trait comes from our new graph crate and we don't own Arc?
There was a problem hiding this comment.
@jsirois : Bingo. Coherence.
Will add a comment.
src/rust/engine/src/scheduler.rs
Outdated
| fn create(&self, entry_id: EntryId) -> Context { | ||
| Context::new(entry_id, self.clone()) | ||
| #[derive(Clone)] | ||
| struct RootContext { |
There was a problem hiding this comment.
Is this because the NodeContext trait comes from our new graph crate and we don't own Arc?
Problem
#4558 involves some relatively fidgety logic, but
Graphwas built in-situ and does not have any tests of its own.Solution
Extract and genericize
Graphto allow for adding unit tests of the new behaviour in #4558. Add a very basic test; more useful ones will follow.