Context assigns branch configuration #339

JakeGinnivan · 2015-01-07T17:08:35Z

No description provided.

JakeGinnivan · 2015-01-07T17:16:11Z

JakeGinnivan · 2015-01-08T12:23:51Z

@nulltoken you might know better than me about this. We seem to have a race condition in commit counting. Rerunning that test which is failing sometimes fails and sometimes passes..

Could there be an issue with commits and the merge commit having the same timestamp?

JakeGinnivan · 2015-01-08T12:24:34Z

GitVersionCore.Tests.IntegrationTests.GitFlow.SupportBranchScenarios.SupportIsCalculatedCorrectly was failing until a rebuild

nulltoken · 2015-01-08T14:11:23Z

@JakeGinnivan If you're referring to DevelopBasedVersionFinderBase.NumberOfCommitsOnBranchSinceCommit(), I don't think so.

Could there be an issue with commits and the merge commit having the same timestamp?

I hope not. Actually relying on timestamps only would be weak. We should always account for the topology (parent/child relationships between commits).

How about surrounding the test code with a tight loop in order to see if we can make it fail more frequently? We may get some more actionable results from a ton of failures.

JakeGinnivan · 2015-01-08T14:48:54Z

@nulltoken doesn't fail that often.. but is intermittent.

System.Exception : Test failed 2/100 times with errors:


    ExecuteGitVersion(repository).ToString("f")
        should be
    "2.1.0-unstable.0+0"
        but was
    "2.1.0-unstable.1+1"


    ExecuteGitVersion(repository).ToString("f")
        should be
    "2.1.0-unstable.0+0"
        but was
    "2.1.0-unstable.1+1"

nulltoken · 2015-01-08T14:52:32Z

@JakeGinnivan Are you able to Console Out the result of the above method as well and see if that correlates?

JakeGinnivan · 2015-01-08T14:55:30Z

@nulltoken nothing relevant is outputted..

Created git repository at 'C:\Users\jake.ginnivan\_code\GitVersion\GitVersionCore.Tests\bin\Debug\TestRepositories\f4151a15-b124-447c-b05a-a30eb692478c'
Running against branch: develop
GitFlow version strategy will be used
Running against branch: support/1.0.0
GitFlow version strategy will be used
Running against branch: release/1.2.0
GitFlow version strategy will be used
Running against branch: support/1.0.0
GitFlow version strategy will be used
Running against branch: hotfix/1.2.1
GitFlow version strategy will be used
Running against branch: support/1.0.0
GitFlow version strategy will be used

x100

        var versionOnMasterFinder = new VersionOnMasterFinder();
        var tip = context.CurrentCommit;

        var f = new CommitFilter
        {
            Since = tip,
            Until = context.Repository.FindBranch("master").Tip,
            SortBy = CommitSortStrategies.Topological | CommitSortStrategies.Time
        };

        var c = context.Repository.Commits.QueryBy(f);
        var numberOfCommitsSinceRelease = c.Count();

This is the commit counting code, for some reason we sometimes get an incorrect count. Normally it returns 0, but sometimes it returns 1..

JakeGinnivan · 2015-01-08T15:21:15Z

Progress

*   commit 79e7cdf (HEAD, tag: 2.0.0, master, develop)
|\  Merge: ae0f676 2dec116
| | Author: A. U. Thor <[email protected]>
| | Date:   0 seconds ago
| | 
| |     Merge branch 'release-2.0.0'
| |   
| * commit 2dec116 (release-2.0.0)
| | Author: A. U. Thor <[email protected]>
| | Date:   0 seconds ago
| | 
| |     Test Commit
| |   
| * commit 2b82b01
|/  Author: A. U. Thor <[email protected]>
|   Date:   0 seconds ago
|   
|       Test Commit
|  
* commit ae0f676 (tag: 1.1.0)
  Author: A. U. Thor <[email protected]>
  Date:   0 seconds ago




*   commit f162942 (HEAD, develop)
|\  Merge: 5ad6616 e5fedc9
| | Author: A. U. Thor <[email protected]>
| | Date:   0 seconds ago
| | 
| |     Merge branch 'release-2.0.0'
| |       
| | *   commit 40fcaaa (tag: 2.0.0, master)
| | |\  Merge: 5ad6616 e5fedc9
| |/ /  Author: A. U. Thor <[email protected]>
|/| /   Date:   1 second ago
| |/    
| |         Merge branch 'release-2.0.0'
| |   
| * commit e5fedc9 (release-2.0.0)
| | Author: A. U. Thor <[email protected]>
| | Date:   1 second ago
| | 
| |     Test Commit
| |   
| * commit c87bb9c
|/  Author: A. U. Thor <[email protected]>
|   Date:   1 second ago
|   
|       Test Commit
|  
* commit 5ad6616 (tag: 1.1.0)
  Author: A. U. Thor <[email protected]>
  Date:   1 second ago

      Test Commit

Extra commit!

JakeGinnivan · 2015-01-08T15:21:35Z

How does that even happen...

JakeGinnivan · 2015-01-08T15:22:34Z

@nulltoken if you are not subscribed to thread.. check above

JakeGinnivan · 2015-01-08T15:24:19Z

@nulltoken Looks like head has not properly moved when tag is applied. Or tag is creating a new commit somehow.

JakeGinnivan · 2015-01-08T15:24:39Z

Graph was produced with

public void DumpGraph()
{
    var output = new StringBuilder();

    ProcessHelper.Run(
        o => output.AppendLine(o),
        e => output.AppendLineFormat("ERROR: {0}", e),
        null,
        "git",
        @"log --graph --abbrev-commit --decorate --date=relative --all",
        RepositoryPath);

    Console.Write(output.ToString());
}

nulltoken · 2015-01-08T16:21:03Z

@JakeGinnivan I'm already watching this thread 😉

So the problem isn't related to the way we count,.

nulltoken · 2015-01-08T16:35:00Z

I think I got it. This is indeed a race concurrency issue, but at the git level.

If, within the same second, one creates two commit, with the same parents, the same message and the same tree, they will both bear the same sha.

In a real life scenario, the second graph would be the expected one (two different merge commits).

If you want to enforce and make the test bulletproof (without thread.sleeping for one second), I think that adding one commit on develop at the very beginning of the test would be enough. This way the merge commits will always bear different parents.

JakeGinnivan · 2015-01-08T16:55:04Z

Magic!

BaseGitFlowRepositoryFixture now creates a commit on develop after branching, test reliably passes.

Tree has changed to

*   commit 0123d14 (HEAD, develop)
|\  Merge: c9d130e 44e9c2b
| | Author: A. U. Thor <[email protected]>
| | Date:   0 seconds ago
| | 
| |     Merge branch 'release-2.0.0'
| |       
| | *   commit f38eb04 (tag: 2.0.0, master)
| | |\  Merge: 6e51d11 44e9c2b
| | |/  Author: A. U. Thor <[email protected]>
| |/|   Date:   0 seconds ago
| | |   
| | |       Merge branch 'release-2.0.0'
| | |    
| * | commit 44e9c2b (release-2.0.0)
| | | Author: A. U. Thor <[email protected]>
| | | Date:   0 seconds ago
| | | 
| | |     Test Commit
| | |    
| * | commit 5bb8e98
|/ /  Author: A. U. Thor <[email protected]>
| |   Date:   0 seconds ago
| |   
| |       Test Commit
| |   
* | commit c9d130e
|/  Author: A. U. Thor <[email protected]>
|   Date:   0 seconds ago
|   
|       Test Commit
|  
* commit 6e51d11 (tag: 1.1.0)
  Author: A. U. Thor <[email protected]>
  Date:   0 seconds ago

      Test Commit

JakeGinnivan · 2015-01-08T16:56:56Z

Which doesn't really look right. but think that it is git log issue.

…s#339 (comment) for info

nulltoken · 2015-01-08T18:32:09Z

Have you chased down all the potential similar issues? Maybe one way to put them under the light would be add a temporary Thread.Sleep(1000) in MergeNoFF().

JakeGinnivan · 2015-01-08T19:14:42Z

Appears ok with the Thread.Sleep in there. Found another test which already fixed that race condition.

Wondering if we should thread.Sleep in MergeNoFF - or somehow detect the condition which could cause it and throw or thread.sleep on demand?

nulltoken · 2015-01-08T19:28:44Z

I tend to not feel at ease with Thread.Sleep() calls as they make the tests slower.

Regarding the detection of the root cause condition, I'm not sure there's a magic answer.

The way I've worked until now is by creating my test case by hand in git command line and then ensuring that the automated ones were reproducing the same topology.

I've more than once relied on the hack of adding an additional commit on a branch (to force the merge sha to differ). Most of the time, that wasn't hacking the test per se as it often makes sense to have a develop branch ahead of master, for instance.

JakeGinnivan · 2015-01-08T19:32:12Z

Yeah, I put it in the GitFlow base fixture, so as long as we use those base fixtures it should be fine.

JakeGinnivan · 2015-01-08T19:32:53Z

So after all that, @yannisgu can you have a look at the PR

yannisgu · 2015-01-08T19:48:47Z

GitVersionCore.Tests/Fixtures/BaseGitFlowRepositoryFixture.cs

+        Repository.MakeACommit();
+    }
+
+    public void DumpGraph()


This was only for test purpose, not?

yep, test purposes. But is useful if we need to debug issues in the future

yannisgu · 2015-01-08T19:57:35Z

Ok seems good 👍

Maybe a small test would be helpfull?

JakeGinnivan · 2015-01-08T19:59:32Z

During the course of this PR tests were failing.

Also planning more changes which will require tests. This was more a refactoring step.

Did you see I took your PR and turned it into a branch in the main repo. This PR is targeting that. If you have further changes also submit PRs to that branch then we can build it up in pieces.

…ration Context assigns branch configuration

yannisgu · 2015-01-08T20:01:26Z

OK

Yes I have seen that, sounds good to me!

JakeGinnivan added 3 commits January 8, 2015 11:57

Current branch configuration should be set by the context

67be1f6

Should match branch name only at start of line

3ecf6fa

Fixed failing tests

3ad8b33

Fixing tests which were affected by a git race condition. See GitTool…

3705f6f

…s#339 (comment) for info

JakeGinnivan force-pushed the ContextAssignsBranchConfiguration branch from 50247e3 to 3705f6f Compare January 8, 2015 17:07

Found a workaround which is fixed now

e7a5548

yannisgu reviewed Jan 8, 2015
View reviewed changes

Fixed issue found in review

e885ce7

JakeGinnivan added a commit that referenced this pull request Jan 8, 2015

Merge pull request #339 from JakeGinnivan/ContextAssignsBranchConfigu…

9fd17dd

…ration Context assigns branch configuration

JakeGinnivan merged commit 9fd17dd into GitTools:BranchSpecificConfiguration Jan 8, 2015

JakeGinnivan deleted the ContextAssignsBranchConfiguration branch January 8, 2015 19:59

Context assigns branch configuration #339

Context assigns branch configuration #339

Uh oh!

Conversation

JakeGinnivan commented Jan 7, 2015

Uh oh!

JakeGinnivan commented Jan 7, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

nulltoken commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

nulltoken commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

nulltoken commented Jan 8, 2015

Uh oh!

nulltoken commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

nulltoken commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

nulltoken commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

yannisgu Jan 8, 2015

Choose a reason for hiding this comment

Uh oh!

JakeGinnivan Jan 8, 2015

Choose a reason for hiding this comment

Uh oh!

yannisgu commented Jan 8, 2015

Uh oh!

JakeGinnivan commented Jan 8, 2015

Uh oh!

yannisgu commented Jan 8, 2015

Uh oh!

Uh oh!