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

Extract and genericize Graph for easier testing #6010

Merged
merged 6 commits into from Jun 26, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Jun 23, 2018

Problem

#4558 involves some relatively fidgety logic, but Graph was built in-situ and does not have any tests of its own.

Solution

Extract and genericize Graph to allow for adding unit tests of the new behaviour in #4558. Add a very basic test; more useful ones will follow.

@stuhood stuhood requested review from baroquebobcat , jsirois and illicitonion Jun 23, 2018

@stuhood stuhood force-pushed the twitter:stuhood/extract-graph branch from b1b4a88 to 775e8cb Jun 23, 2018

@stuhood stuhood force-pushed the twitter:stuhood/extract-graph branch from 868cb58 to 9358577 Jun 24, 2018

@stuhood stuhood requested review from cosmicexplorer and removed request for jsirois Jun 24, 2018

fn format(&self) -> String;

///
/// If this Node represents an FS operation, returns its input Path.

This comment has been minimized.

@illicitonion

illicitonion Jun 25, 2018

Contributor

What kind of path? Absolute? Relative to build root?

This comment has been minimized.

@stuhood

stuhood Jun 25, 2018

Member

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.

This comment has been minimized.

@illicitonion

illicitonion Jun 25, 2018

Contributor

What does bottom mean? A leaf node? The most recently executed stack frame? The opposite of one of the above?

fn create(&self, entry_id: EntryId) -> Context {
Context::new(entry_id, self.clone())
#[derive(Clone)]
struct RootContext {

This comment has been minimized.

@illicitonion

illicitonion Jun 25, 2018

Contributor

What value is this providing over just using an Arc<Core>?

This comment has been minimized.

@jsirois

jsirois Jun 25, 2018

Member

Is this because the NodeContext trait comes from our new graph crate and we don't own Arc?

This comment has been minimized.

@stuhood

stuhood Jun 25, 2018

Member

@jsirois : Bingo. Coherence.

Will add a comment.

fn create(&self, entry_id: EntryId) -> Context {
Context::new(entry_id, self.clone())
#[derive(Clone)]
struct RootContext {

This comment has been minimized.

@jsirois

jsirois Jun 25, 2018

Member

Is this because the NodeContext trait comes from our new graph crate and we don't own Arc?

@stuhood stuhood merged commit 48224fc into pantsbuild:master Jun 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/extract-graph branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment