tiny little typo: "a ny"

thank @yeraydavidrodriguez I will open the issue again and fix it.

Fixed with #259

I'm not sure about it, but here we are testing two inter-dependent things in the same test: that 'git log' do not fail when there is a commit and that the emptyCommit actually creates the commit.
What if emptyCommit creates the commit but not as we want? Maybe we should test it independently.

hi @yeraydavidrodriguez you are totally right. I do not like it either.

In general, I do not think it's a problem to use other classes or the class you are testing in your tests. Especially if you want to reach a complex state for the class you want to test. For example, you have to use the constructor and the constructor could have a bug and make your test fail. Or you could need some initialization before calling the method you want to test.

But in this case, I agree the test would be better if we create the state with an external helper. And not only for the commit but for the rest of the needed steps: init, committer configuration and commit. In fact, there is something even worse, those new methods: init, setLocalConfig and emptyCommit are only used in tests.

The reason I kept it that way is:

• I think in the long term, those methods could be useful also for production code.
• Sometimes I add methods to production classes in order to improve their testability.

I suppose the right way to do it would have been:

Keep the production class simple:

export class Git {
  private readonly dir: GitRepoDir

  constructor(dir: GitRepoDir {
    this.dir = dir
  }

  execSync(args?: readonly string[]): string {
    const cmd = `git`
    const options: ExecFileSyncOptions = {
      stdio: 'pipe',
      shell: false,
      cwd: this.dir.getDirPath(),
      ...(typeof this.env !== 'undefined' && {env: this.env})
    }

    return execFileSync(cmd, args, options).toString()
  }

  status(): string {
    const args: readonly string[] = ['status']
    return this.execSync(args)
  }

  log(): string {
    const args: readonly string[] = ['log', '-n', '0']
    return this.execSync(args)
  }
}

And create a new helper class for testing:

export class GitWrapper {
  private readonly dir: GitRepoDir
  private readonly env: NodeJS.ProcessEnv | undefined

  constructor(dir: GitRepoDir, env?: NodeJS.ProcessEnv | undefined) {
    this.dir = dir
    this.env = env
  }

  execSync(args?: readonly string[]): string {
    const cmd = `git`
    const options: ExecFileSyncOptions = {
      stdio: 'pipe',
      shell: false,
      cwd: this.dir.getDirPath(),
      ...(typeof this.env !== 'undefined' && {env: this.env})
    }

    return execFileSync(cmd, args, options).toString()
  }

  init(): string {
    const args: readonly string[] = ['init']
    return this.execSync(args)
  }

  emptyCommit(message: string): string {
    const args: readonly string[] = [
      'commit',
      '--allow-empty',
      '-m',
      `"${message}"`
    ]
    return this.execSync(args)
  }

  setLocalConfig(key: string, value: string): string {
    const args: readonly string[] = ['config', `${key}`, `"${value}"`]
    return this.execSync(args)
  }
}

We could use this Git even more simple wrapper to build the status we want to test in a Git repo. If at some point we need those methods in the production class we can copy them.

In fact, we could write a builder to do things like:

const gitStatusBuilder: GitWrapper = new GitStatusBuidler().
  .fromNewEmptyDir().
  .init().
  .setLocalConfig("user.name", "A Committer")
  .setLocalConfig("user.email", "committer@email.com")
  .emptyCommit("message").
  .build()

Internally would do:

cd /tmp
mkdir random_name
cd random_name
git init
git commit --allow-empty -m "message"

A very nice approach that last one. Thanks for the clarifications.

hi @yeraydavidrodriguez in the end I realized that Simple-Git has a raw method to execute custom git commands. I'm using that method instead of adding a new Git wrapper.

Changes are implemented in this PR #259

• Here
• And here

Regarding the GitStatusBuidler we do not have any complex scenario right now. FOr now, I'm also using the raw method. See here.

This is the sample test where I need to create a new commit:

  it('should check if an initialized repo has commits', async () => {
    const gitRepoDir = new GitRepoDir(await createTempEmptyDir())
    const git = await newSimpleGitWithCommitterIdentity(gitRepoDir)

    const gitRepo = new GitRepo(gitRepoDir, git)
    await gitRepo.init()

    expect(await gitRepo.hasCommits()).toBe(false)

    await git.raw(
      'commit',
      '--no-gpg-sign',
      '--allow-empty',
      '--message',
      'Initial commit'
    )

    expect(await gitRepo.hasCommits()).toBe(true)
  })

If we need to create more complex git repo states for testing we can create the GitStatusBuidler but using also Simple-Git.

Great, @josecelano , it seems a good choice to avoid adding unnecessary complexity layers.