/wait

@envoyproxy/senior-maintainers assignee is @phlax

Assigning Harvey for a senior-maintainer pass.
/assign @htuch

Waiting for comment update.
/wait

@htuch , I removed the test case I added in last commit. Because in the CI test environment, there is no /sysroot directory, so the function canonicalPath can not return a valid path so that /sysroot will always be considered as illegal, the test case will fail.

EXPECT_FALSE(file_system_.illegalPath("/sysroot/"));

In my local system, I have the /sysroot directory, the test case will pass.
I do not have any good ideas to add the test cases in CI environment except the following way

  const Api::SysCallStringResult canonical_path = canonicalPath("/sysroot");
  if (canonical_path.return_value_.empty()) {
    EXPECT_TRUE(file_system_.illegalPath("/sysroot")); 
  } else {
    EXPECT_FALSE(file_system_.illegalPath("/sysroot")); 
  }

But I do not think it is necessary to have above test code. What's your opinion?

I think the whole test should move to a mock environment for filesystem impl, to avoid the CI machine dependency. This probably will save some grief in the future. We could leave one or two tests using the real /dev maybe, which should be standard, and mock the rest.

Concretely what this means here is that in the canonicalPath implementation, we shouldn't use ::realpath directly, but some variant in os_syscalls_impl.cc that can be mocked out by replacing the OS syscall singleton

If we need to refactor the whole testing for filesystem impl, can we have another PR to do that?

@zhangbo1882 that's fine if you're willing to do that.

I will try my best to do that. Do we need to create a issue for that ?