Implement composite Git strategy by BinaryMuse · Pull Request #538 · atom/github

BinaryMuse · 2017-02-18T16:26:35Z

⚠️ Here there be 🐲s (and even scarier: metaprogramming! 😱) ⚠️

#524 experiments with utilizing nodegit for some operations; in order to more easily modify and switch between implementations (perhaps even dynamically at runtime), it'd be nice to have both implementations in the codebase in a fully complete state at all times. This PR reverts GitShellOutStrategy to use pure shelling out and adds NodegitStrategy which implements some operations using nodegit.

Since the nodegit strategy doesn't implement all the Git operations, this PR also introduces a new helper called firstImplementer which takes an array of receivers and returns an object that intercepts property accessors and returns the property value from the first receiver that contains that property. An example may help:

class A {
  one()   { return 'a-one'; }
  two()   { return 'a-two'; }
}

class B {
  two()   { return 'b-two'; }
  three() { return 'b-three'; }
}

const target = firstImplementer(new A(), new B());
target.one()   // a-one
target.two()   // a-two
target.three() // b-three

A new strategy called CompositeGitStrategy uses this helper to automatically create both the underlying Git strategies, creates a shared async queue for both of them, and routes method calls to the appropriate strategy as approprite.

The GitShellOutStrategy tests have been converted to GitStrategiesTest, which tests all strategies, and which uses even more magic to only test the methods that are implemented on the strategy under test. There is some complexity here because even when we're testing the nodegit strategy, we often need methods the strategy doesn't implement for supporting calls (like calling exec to set local config). So, the tests are implemented using a special version of the composite strategy which ensures that the method under test is called on the strategy under test, but lets other method calls be fulfilled by either strategy. This code is under heavy construction. 🚧

The other downside to this... er, strategy, is that it's implemented via a Proxy, and since sinon does it's own metaprogrammy stuff to create stubs and mocks, they're not playing super well together. I'm still working on this as well.

This replaces the shelling out version (which was writing to the index) with a read-only implementation, allowing us to parallelize this operation and improve performance 🐎

smashwilson

This is super cool and a big validation of the Strategy pattern for git operations 🎉

The metaprogramming isn't nearly as scary as advertised, either; I've done worse myself in production code 😉 I'd recommend maybe a few more comments to document it maybe, especially around the tests?

smashwilson · 2017-02-20T19:27:21Z


 import {GitProcess} from 'git-kitchen-sink';
 import {parse as parseDiff} from 'what-the-diff';
-import Git from 'nodegit';


I was actually going to comment on the inappropriateness of the name GitShellOutStrategy in #524, by the way 😆

smashwilson · 2017-02-20T19:29:47Z

+ * Takes an array of targets and returns a proxy. The proxy intercepts property accessor calls and
+ * returns the value of that property on the first object in `targets` where the target implements that property.
+ */
+export function firstImplementer(...targets) {


This isn't nearly as scary as you made it sound 😉

smashwilson · 2017-02-20T19:31:02Z

+      const STATUS = Git.Status.STATUS;
+      const index = await repo.index();
+
+      /* eslint-disable no-bitwise */


That's kind of a weird eslint rule. I guess bitwise operations are rare enough that they assume it's a typo for boolean operations?

smashwilson · 2017-02-20T19:35:27Z

+    function createTestStrategyForPropertyUnderTest(...args) {
+      const compositeStrategy = CompositeGitStrategy.create(...args);
+      const originalStrategy = new Strategy(...args);
+      return new Proxy(compositeStrategy, {


Okay, this bit gets a little hairy 😆 I think I still follow, though.

smashwilson · 2017-02-20T19:36:26Z

+        operations.forEach(op => {
+          it(`temporarily overrides gpg.program when ${op.progressiveTense}`, async () => {
+            // if (op !== 'pulling') { return; }
+            console.log('========================');


Ah yes — the pull tests were failing (still are IIRC) and I was trying to figure out why

smashwilson

I definitely prefer the chained methods to the single method with four parameters 👍 Much more readable.

smashwilson · 2017-02-22T19:45:02Z

+        });
+      } else {
+        return describeMethod(description, function() {
+          it('is not implemented', function() {});


Hmm yeah I guess it was misleading to have these as "pending" tests, considering that we wouldn't actually intend to implement many of them anyway.

smashwilson · 2017-02-22T19:45:05Z

+
+[GitShellOutStrategy, NodeGitStrategy].forEach(function(Strategy) {
+  forStrategy(Strategy, function(forCommand) {
+    describe.only(`Git commands for ${Strategy.name}`, function() {


🔥 .only() 😉

smashwilson · 2017-02-22T19:45:38Z

+        });
+
+        it('displays copied files as an added file', async function() {
+        // This repo is carefully constructed after much experimentation


Indentation looks a touch off on these lines 🎨

…rategy

…trategy

BinaryMuse · 2017-02-23T01:21:47Z

Since #524 is closed, I've set this PR's base to master. I'm keeping CompositeGitStrategy as it's a great way to experiment with new implementations of some features, and the testing story here is relatively complete.

BinaryMuse · 2017-02-23T01:53:44Z

+    getPrototypeOf(target) {
+      return targets.reduce((acc, t) => {
+        return Object.create(acc, descriptorsFromProto(Object.getPrototypeOf(t)));
+      }, Object.prototype);


This was surprisingly tricky to work out! Huge thanks to @kuychaco who rocked this.

BinaryMuse · 2017-02-23T01:54:35Z

+].forEach(function(strategies) {
+  const createTestStrategy = (...args) => {
+    return CompositeGitStrategy.withStrategies(strategies)(...args);
+  };


@smashwilson As I was reviewing this with @kuychaco, she recommended this much simpler (and much improved, IMO) strategy. Thoughts?

Hmm! Definitely much cleaner.

Let's see. We lose the ability to have the test suite identify which methods are implemented on each strategy... but we gain higher fidelity to the way the strategies will actually be used by the package. 👍

To go even further - in what situation would we want to run the suite with different combinations of strategies in one run? Presumably the package will use a fixed strategy configuration at any given time, so as long as we stay consistent with that, these tests will exercise what they're supposed to exercise. Maybe we don't even need the outer loop!

@smashwilson That's a really good question! And you're right, in the end we'll only use a specific order for the production strategy. The point of these tests was specifically to unit test each individual strategy's methods as much as possible; however, to do so, we may need to use methods that only exist on the other strategy (such as git.exec to set configs or do other test setup). That's why each isn't tested in complete isolation, but as part of a composite strategy.

BinaryMuse · 2017-02-23T01:55:38Z

+          action: () => git.pull('some-branch'),
+          configureStub: (stub, gitStrategy) => {
+            sinon.stub(gitStrategy, 'getRemoteForBranch').returns(Promise.resolve('origin'));
+          },


@smashwilson We also changed this up a bit.

@kuychaco

@kuychaco was right

BinaryMuse and others added 13 commits February 15, 2017 15:59

Let's do some nodegits

b212aab

Use nodegit for GSOS#getStatusesForChangedFiles

f064145

This replaces the shelling out version (which was writing to the index) with a read-only implementation, allowing us to parallelize this operation and improve performance 🐎

Only select item in staging view when clicked or dragged over

cfd9b49

👕

4049b12

Convert GSOS#getRemotes to nodegit

49a2fce

Ensure tooltip is closed if showing when component unmounts

3c02de8

Add firstImplementer helper

79a90fe

Revert GSOS to original (pre-nodegit) form

82e2932

Allow an AsyncQueue to be injected into GSOS

2a8d23a

Implement set/getOwnPropertyDescriptor/getPrototypeOf in firstImpementer

7085c7e

Extract Nodegit strategy

99dbc5a

Use CompositeGitStrategy as default strategy

a0be11e

Convert GSOS tests to generic strategies test

14c9cbe

smashwilson reviewed Feb 20, 2017

View reviewed changes

sdescribe(cmd, desc) -> forCommand(cmd).describe(desc)

57887e3

BinaryMuse force-pushed the mkt-composite-git-strategy branch from 8463f26 to 57887e3 Compare February 22, 2017 19:36

smashwilson reviewed Feb 22, 2017

View reviewed changes

Michelle Tilley and others added 13 commits February 22, 2017 12:22

Return assignment from Proxy set

d1b156c

Make getRemotes test work independent of order

c0d2b1d

Redirect ALL methods that exist on the strategy under test to that st…

0443f6a

…rategy

Show non-implemented strategy tests as pending

cac1741

🎨

72b1d00

🔥 .only

3d0ec93

🎨

2bc7061

Ensure proper returns from firstImplementer

1495e5a

Always test every method on every strategy

75f1e81

Only test gpgExec on strategies where it actually exists

313300c

Implement getPrototypeOf in firstImplementer correctly

450b0ca

Change the way strategies get tested

2ab6105

🎨

410525c

Drop the nodegit strategy like a hot potato 🍠

68d7995

BinaryMuse changed the base branch from mkt-ku-nodegit2-the-revenge-of-nodegit to master February 23, 2017 01:19

Merge remote-tracking branch 'origin/master' into mkt-composite-git-s…

d588ec2

…trategy

Michelle Tilley added 2 commits February 22, 2017 17:50

👕

6611be9

🎨

c840e26

BinaryMuse commented Feb 23, 2017

View reviewed changes

🔥 unused code

84b6575

BinaryMuse commented Feb 23, 2017

View reviewed changes

Michelle Tilley added 3 commits February 22, 2017 21:44

Be more specific with firstImplementer tests

d50cce6

Use reduceRight to produce correct prototype

17308d3

@kuychaco was right

Test firstImplementer set and getOwnPropertyDescriptor

40d9a6f

smashwilson added this to the Public Release milestone Feb 23, 2017

BinaryMuse merged commit 6c0565f into master Feb 23, 2017

BinaryMuse deleted the mkt-composite-git-strategy branch February 23, 2017 19:42

Conversation

BinaryMuse commented Feb 18, 2017

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BinaryMuse commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants