Fix a bug with setting test folders in ContentEntryEditor

[kamildoleglo]

Checklist

• [x] I have filed an issue about this change and discussed potential changes with the maintainers.
• [ ] I have received the approval from the maintainers to make this change.
• [x] This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See the Contributions section in the README for more details.

Discussion thread for this change

Issue number: 3670

Description of this change

If a test folder is not conforming to any regex in the project view file but is marked as a test source by the SourceFolderProvider, then the setSourceFolderForLocation method is called on the relevant SourceFolderProvider. However, if the provider then adds the folder as a test source anyway, the ContentEntryEditor removes it in the subsequent step, because the ContentEntry map has the (folder, isTestSource) pair as the unique key. This prevents the SourceFolderProviders from setting the test source folders

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[tpasternak]

Hey Kamil, could you share an example showing the issue?

[tpasternak]

However, if the provider then adds the folder as a test source anyway

And when can this happen?

[tpasternak]

The code above is essentially equivalent to the one below.

I'm wondering why we don't contentEntry.removeSourceFolder(parent) here.

Apart from this, I guess you have some custom provider which sets isTest==true. In order to handle such cases, we would probably have to handle removal of source folders inside provider's implementation

    SourceFolder current = sourceFolders.get(new File(file.getPath()));
    SourceFolder next;
    if (current != null) {
      if (isTest != current.isTestSource()) {
        next = provider.setSourceFolderForLocation(contentEntry, current, file, isTest);
        contentEntry.removeSourceFolder(current);
      } else {
        next = current;
      }
    } else {
      if (parent != null && isTest != parent.isTestSource()) {
        next = provider.setSourceFolderForLocation(contentEntry, parent, file, isTest);
      } else {
        next = parent;
      }
    }
    for (Map.Entry<WorkspacePath, DirectoryStructure> child :
        directoryStructure.directories.entrySet()) {
      walkFileSystem(
          workspaceRoot,
          testConfig,
          excludedDirectories,
          contentEntry,
          provider,
          sourceFolders,
          next,
          child.getKey(),
          child.getValue());
    }

[tpasternak]

So the intention is to let providers add test source folders even if they are not specified in projectview's test_sources

1. Extract the code to a simpler form, without currentOrParent variable
2. Move removeTestSources to provider's interface, as a new method with default implementation, and call this in place of contentEntry.removeSourceFolder
3. Provide default implementation for the new method

This will keep the old behavior, but allow provider implementations to skip the remove step

[tpasternak]

Or we may even do item 1 in a separate PR

[sgowroji]

Hello @kamildoleglo, Could you please reply to above comments, So that we can push this PR for further good state. Thanks!

[kamildoleglo]

Currently done item 1. in https://github.com/bazelbuild/intellij/pull/3832 and responded with some additional context in https://github.com/bazelbuild/intellij/issues/3670