It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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).

For more information, open the CLA check for this pull request.

Thanks for the contribution!

I encountered this problem and there doesn't seem to be an related issue, so I'll just describe my issue here.
Should I open an issue first and then create this pull request?

Yes, please do open an issue with details, and then link it from here.

Yes, please do open an issue with details, and then link it from here.

Sorry for my substandard pr.
I have now created an issue and linked it, do I need to do anything else?

I have now created an issue and linked it, do I need to do anything else?

The checklist in the PR description has all of the required steps for a PR. I'm going to mark this as a draft for now, and once you've worked through the checklist please mark it as ready for review. Thanks!

The checklist in the PR description has all of the required steps for a PR. I'm going to mark this as a draft for now, and once you've worked through the checklist please mark it as ready for review. Thanks!

Hi, I am trying to write a test for the current use case, and I found that the existing tests are using a fake window, but in the current use case, it seems that the cache of the default folder is not generated because a dialog is not really opened, so the above problem cannot be reproduced.

Should I try to actually open a Dialog to complete the test case (it might block the ci test, I don't know if there is a better way)

Here is my test code that does not work:

TEST(FileSelectorPlugin, TestInitialDirectoryOfSecondTimeOpen) {
  // First time
  const HWND fake_window = reinterpret_cast<HWND>(1337);
  ScopedTestShellItem fake_selected_file;

  std::unique_ptr<MockMethodResult> result =
      std::make_unique<MockMethodResult>();

  bool shown = false;
  MockShow show_validator =
      [&shown, fake_result = fake_selected_file.file(), fake_window](
          const TestFileDialogController& dialog, HWND parent) {
        shown = true;
        EXPECT_EQ(parent, fake_window);

        // Validate arguments.
        EXPECT_EQ(dialog.GetDefaultFolderPath(), L"C:\\Program Files");
        EXPECT_EQ(dialog.GetOkButtonLabel(), L"Save it!");

        return MockShowResult(fake_result);
      };
  EncodableValue expected_path(Utf8FromUtf16(fake_selected_file.path()));
  // Expect the mock path.
  EXPECT_CALL(*result, SuccessInternal(Pointee(expected_path)));

  FileSelectorPlugin plugin(
      [fake_window] { return fake_window; },
      std::make_unique<TestFileDialogControllerFactory>(show_validator));
  plugin.HandleMethodCall(
      MethodCall(
          "getSavePath",
          std::make_unique<EncodableValue>(EncodableMap({
              // This directory must exist.
              {EncodableValue("initialDirectory"),
               EncodableValue("C:\\Program Files")},
              {EncodableValue("confirmButtonText"), EncodableValue("Save it!")},
          }))),
      std::move(result));

  EXPECT_TRUE(shown);

  // Second time
  std::unique_ptr<MockMethodResult> result_2 =
      std::make_unique<MockMethodResult>();

  shown = false;
  show_validator =
      [&shown, fake_result = fake_selected_file.file(), fake_window](
          const TestFileDialogController& dialog, HWND parent) {
        shown = true;
        EXPECT_EQ(parent, fake_window);

        // Validate arguments.
        EXPECT_EQ(dialog.GetDefaultFolderPath(), L"C:\\Program Files (x86)");
        EXPECT_EQ(dialog.GetOkButtonLabel(), L"Save it!");

        return MockShowResult(fake_result);
      };
  EncodableValue expected_path_2(Utf8FromUtf16(fake_selected_file.path()));
  // Expect the mock path.
  EXPECT_CALL(*result_2, SuccessInternal(Pointee(expected_path_2)));

  FileSelectorPlugin plugin_2(
      [fake_window] { return fake_window; },
      std::make_unique<TestFileDialogControllerFactory>(show_validator));
  plugin_2.HandleMethodCall(
      MethodCall(
          "getSavePath",
          std::make_unique<EncodableValue>(EncodableMap({
              // This directory must exist.
              {EncodableValue("initialDirectory"),
               EncodableValue("C:\\Program Files (x86)")},
              {EncodableValue("confirmButtonText"), EncodableValue("Save it!")},
          }))),
      std::move(result_2));

  EXPECT_TRUE(shown);
}

Hi, I am trying to write a test for the current use case, and I found that the existing tests are using a fake window, but in the current use case, it seems that the cache of the default folder is not generated because a dialog is not really opened, so the above problem cannot be reproduced.

Currently (see below) it's sufficient to validate in tests that we are calling the correct APIs. If you update the wrapper class to have a wrapper for the correct API instead of the one it's currently using (you should not be doing anything other that direct passthroughs to the API with the same name; see

Should I try to actually open a Dialog to complete the test case (it might block the ci test, I don't know if there is a better way)

You should not actually bring up the modal dialog. Doing that cleanly would require flutter/flutter#70233, which we don't currently have support for. So while checking the actual folder in the actual dialog would be the best test here, we're limited to unit tests at the moment.

Currently (see below) it's sufficient to validate in tests that we are calling the correct APIs. If you update the wrapper class to have a wrapper for the correct API instead of the one it's currently using (you should not be doing anything other that direct passthroughs to the API with the same name;

Since I just modified the method of FileDialogController, it seems that I don't need to do any additional testing?🤔 Then refactor the FileDialogController method to name it the same as SetFolder

@tgalpha Are you still planning on updating this to rename the wrapper (and thus the calls to it) per my comments above?

@stuartmorgan I assumed I still needed to write tests for this case, but I can't do it at the moment so I've put it on hold for a while. If I just need to rename the wrapper, I just finished it and committed it.

then making the tests compile again will be sufficient coverage.

Ah, I misremembered the test class details; I thought we were intercepting the SetDefaultFolder call, so I thought that the tests would need to be updated as well when you renamed the method to match what it's now wrapping.

Since we can't test the behavior directly at the moment, what you can do to get some test coverage is to make this do what I thought we were doing already: override the new SetFolder in TestFileDialogController, and have it still do the passthrough to super, but also store the value passed to it, and then add an accessor for that value (I would call it GetSetFolderPath or something, and rename GetDefaultFolderPath to GetDialogFolderPath, and add declaration comments to them explaining the difference).

Then you can update the existing tests that check the folder path to also check that SetFolder was called with the expected value, with a comment in the test explaining why.

It's not ideal, but it would still make it difficult for someone to accidentally break this, since the real FileDialogController is explicitly supposed to be (as its comment says) direct passthroughs only, so nobody should change the behavior of FileDialogController::SetFolder itself, and would thus break the test if they stopped using SetFolder.

@stuartmorgan Thanks for the detailed explanation, I tried to update the tests. But I'm not familiar with C++, would you mind reviewing the code?

• This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.