Experiment Commands #3: Add command for exp gc with selectable flags

[rogermparent]

Based on #268. Part of #229, which is part of #239

This PR adds a single VSCode Command for dvc gc, which invokes a QuickPick menu that allows the user to select any of --all-branches, --all-tags, --all-commits, and --queue.

• Any items the user selects are translated to their respective flags and passed to dvc gc in one command.
• The command always includes dvc exp gc -f -w. This is also the command run when the quickpick is properly submitted with no selected options.
  • -f to skip the CLI warning (if anything, we'll implement our own)
  • -w because it is effectively always enabled, and it only exists so dvc exp gc with no options won't do anything.
• If the QuickPick menu is dismissed with esc, no command is run
• The stdout or stderr of a command will be reported through the showInformationMessage and showErrorMessage functions, respectively.

We could handle this more gracefully by intercepting known errors and displaying more concise messages, but I think that can be considered its own issue that could best be handled with a more unified way of command output reporting.

[mattseddon]

Few questions on this one

[mattseddon]

[N] no need for Dvc

[mattseddon]

[N] use '.'

[mattseddon]

[I] Hoist with other experiment commands

[mattseddon]

[Q] What is the benefit of using a snapshot here?

[mattseddon]

[Q] I see all the other tests have mockedShowQuickPick.mockResolvedValue how does this one get it's resolved value?

[mattseddon]

If you moved the implementation to a constant like so

export const gcQuickPickItems = [
      {
        label: 'All Branches',
        detail: 'Preserve Experiments derived from all Git branches',
        flag: DvcGcPreserveFlag.ALL_BRANCHES
      },
      {
        label: 'All Tags',
        detail: 'Preserve Experiments derived from all Git tags',
        flag: DvcGcPreserveFlag.ALL_TAGS
      },
      {
        label: 'All Commits',
        detail: 'Preserve Experiments derived from all Git commits',
        flag: DvcGcPreserveFlag.ALL_COMMITS
      },
      {
        label: 'Queued Experiments',
        detail: 'Preserve all queued Experiments',
        flag: DvcGcPreserveFlag.QUEUED
      }
    ]

export const gcQuickPickOptions = {
  canPickMany: true,
  placeHolder: 'Select which Experiments to preserve'
}

then you could import that into the test and check that is what is being passed to show quick pick.

You could also use gcQuickPickItems.slice(1,3) for the next test down giving this implementation in the test:

  it('should invoke a QuickPick with the gcQuickPickItems options', async () => {
    await experimentGcCommand(exampleConfig)
    expect(mockedShowQuickPick.mock.calls).toEqual([
      [gcQuickPickItems, gcQuickPickOptions]
    ])
  })

  it('should executes the proper command given a mocked selection', async () => {
    mockedShowQuickPick.mockResolvedValue(gcQuickPickItems.slice(1, 3))

    await experimentGcCommand(exampleConfig)

    expect(mockedExecPromise).toBeCalledWith(
      'dvc exp gc -f -w --all-tags --all-commits',
      {
        cwd: exampleConfig.workspaceRoot
      }
    )
  })

and this in the code:

export const gcQuickPickOptions = {
  canPickMany: true,
  placeHolder: 'Select which Experiments to preserve'
}

export const experimentGcCommand = async (config: Config) => {
  const quickPickResult = (await window.showQuickPick<DvcGcQuickPickItem>(
    gcQuickPickItems,
    gcQuickPickOptions
  )) as undefined | DvcGcQuickPickItem[]

  if (quickPickResult) {
    try {
      const stdout = await experimentGarbageCollect(
        {
          cwd: config.workspaceRoot,
          cliPath: config.dvcPath
        },
        quickPickResult.map(({ flag }) => flag)
      )
      window.showInformationMessage(stdout)
    } catch (e) {
      window.showErrorMessage(e.stderr || e.message)
    }
  }
}

I still think that quick pick functionality should be outsourced from this file.

[rogermparent]

[Q] What is the benefit of using a snapshot here?

This test acts as check that allows us to refactor the experiment GC command in any way and expect the same shape, and with a snapshot a dev can compare changed outputs and deliberately accept the changes with a jest -u.

[Q] I see all the other tests have mockedShowQuickPick.mockResolvedValue how does this one get it's resolved value?

This test doesn't set a resolved value because the resolved value of showQuickPick doesn't matter in this test, just the arguments passed to it. Explicitly defining the mock implementation to return something like undefined would be more explicit, but functionally the exact same.

If you moved the implementation to a constant like so [...] then you could import that into the test and check that is what is being passed to show quick pick.

While that is doable, it would add unnecessary exports and overhead while not adding any benefit over using a snapshot.
Directly comparing an exported argument constant would require the test to be changed around for most implementation changes, while the snapshot holds true regardless of where the QuickPick options come from and how they're build.

I still think that quick pick functionality should be outsourced from this file.

At the risk of sounding like I'm pulling a "no, you!", I think index would be best as a module that handles how the dvc commands interface to VSCode with stuff like quickpicks, and the functions that run the commands be in their own file.

I do agree that the cli dir needs a bit of restructuring, as it's become a bit of a big ball of mud over time.

[mattseddon]

[Q] do you think this would be better placed in another file? This is probably also a better candidate for an integration test.

[rogermparent]

do you think this would be better placed in another file?

Maybe these defined vscode callbacks and the registerCommands function could find a better home in something like a module named commands.

This is probably also a better candidate for an integration test.

I disagree, mocks test this functionality perfectly fine. The integration tests are already quite heavy and often flaky, we should only use them when they're really necessary. Jest tests also provide coverage and are much more easily integrated into all forms of CI.