[Nexthop][fboss2-dev] Add test to cover default constructor of ConfigSession (#526)

[hillol-nexthop]

Summary

There was no way to test the default constructor of ConfigSession in UT. The change is made to extend the coverge.
Implemented environment variable override (FBOSS_CONFIG_BASE_DIR) for AgentDirectoryUtil to enable testing of ConfigSession default constructor.

Changes Made

1. AgentDirectoryUtil.cpp

Added a helper function getBaseConfigDir() that checks for the FBOSS_CONFIG_BASE_DIR environment variable:

2. CmdConfigSessionTest.cpp

Added test defaultConstructorWithEnvOverride that:

• Sets FBOSS_CONFIG_BASE_DIR to a test directory
• Calls ConfigSession::getInstance() which uses the default constructor
• Verifies the session is created with the test paths
• Confirms the default constructor is now fully testable

Benefits

✅ Backward Compatible: Production code unchanged - defaults to /etc/coop if env var not set
✅ Test-Friendly: Tests can now use the default constructor by setting FBOSS_CONFIG_BASE_DIR
✅ No API Changes: Existing code continues to work without modifications
✅ Follows Existing Patterns: Similar to FLAGS_volatile_state_dir and FLAGS_persistent_state_dir

Production Behavior

In production, FBOSS_CONFIG_BASE_DIR is not set, so the default /etc/coop path is used. No changes to production deployment or configuration required.

Pre-submission checklist

• I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
• pre-commit run

Test Plan

The change is primarily to extend testing.

[meta-codesync]

@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D99499050.

[joseph5wu]

Actually do you think instead of asking fboss/agent/AgentDirectoryUtil.cpp to check the system environment, can we actually use the non-default AgentConfigUtil constructor in ConfigSession?
https://github.com/facebook/fboss/blob/main/fboss/agent/AgentDirectoryUtil.h#L34

That's the reason that we have different constructors in this util library. I think if we are changing the default behavior with this extra getenv() logic, it kinda misses the point of having different constructors, right?

[facebook-github-tools]

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

[hillol-nexthop]

Actually do you think instead of asking fboss/agent/AgentDirectoryUtil.cpp to check the system environment, can we actually use the non-default AgentConfigUtil constructor in ConfigSession? https://github.com/facebook/fboss/blob/main/fboss/agent/AgentDirectoryUtil.h#L34

That's the reason that we have different constructors in this util library. I think if we are changing the default behavior with this extra getenv() logic, it kinda misses the point of having different constructors, right?

Updated

[joseph5wu]

Most of our existing config unit tests are using TestableConfigSession, which is not using the default ConfigSession::ConfigSession() which uses the default /etc/coop/ config directory. Instead TestableConfigSession is using ConfigSession::ConfigSession() with its specified sessionConfigDir and systemConfigDir without using AgentDirectoryUtil.
Do we still need this change if current existing unit tests are already not using the default /etc/coop/agent.conf?

[joseph5wu]

@hillol-nexthop The second ConfigSession::ConfigSession() with sessionConfigDir/systemConfigDir is used for TestableConfigSession, which is used by the unit tests
https://github.com/facebook/fboss/blob/main/fboss/cli/fboss2/test/TestableConfigSession.h#L27

And the default ConfigSession::ConfigSession() will be used by the regular fboss2-dev logic?
When you changed the code here instead of the other constructor but your test can still pass, it makes me think we don't even need such change?

[hillol-nexthop]

True. The default constructor is used in the regular fboss2-dev and TestableConfigSession is used in UT. Because of this, the default constructor remains uncovered in the UT. Some times back there was a breakage in the fboss2-dev because of a bug in the default constructor flow (don't remember the exact breakage though!) and then I thought it would be good to have some coverage of the default constructor. And this change is just to make the default constructor testable.
See if it makes sense.

[joseph5wu]

I'm afraid I still didn't see the value of making the change here on the default ConfigSession::ConfigSession() by changing the coop config directory to the one you prefer in UT, which is exactly the second constructor ConfigSession::ConfigSession() is created for.
If you want to make sure both constructors have high test coverage, why not just do something like:

  ConfigSession::ConfigSession()
      : ConfigSession(
            getHomeDirectory() + "/.fboss2",
            fs::path(AgentDirectoryUtil().getConfigDirectory())
                .parent_path()
                .string()) {}

  ConfigSession::ConfigSession(
      std::string sessionConfigDir,
      std::string systemConfigDir)
      : sessionConfigDir_(std::move(sessionConfigDir)),
        systemConfigDir_(std::move(systemConfigDir)),
        username_(getUsername()),
        git_(std::make_unique<Git>(systemConfigDir_)) {
    initializeSession();
  }

So you will know besides the sessionConfigDir_ and systemConfigDir_ are different b.w regular use of fboss2-dev and UT, everything else will have the same implementation

[hillol-nexthop]

The coop dir will depend on the env var- so it remains the same in prod and will change in test.
Your approach looks clean as you are delegating the job to parameterised constructor from the default one and the parameterised constructor is already tested in UT.
But my motivation was to touch the default constructor flow where AgentDirectoryUtil instantiates and creates the path for coop dir because this is where I found issue some times back. You are condensing the existing logic to a single line here but this flow will remain untested.

[benoit-nexthop]

@joseph5wu we can totally have the 2-arg ctor call the default ctor, but it doesn't change anything to the fact that the default ctor will remain untested.

[joseph5wu]

@hillol-nexthop / @benoit-nexthop : Sure if we're chasing 100% test coverage here, then we can add the test via env variables. But this is never gonna be possible to be 100% test coverage in reality.
So now we have to answer the question, what do we really want to test here?
Do you want to test the default AgentDirectoryUtil behavior in this default constructor? But the way you did here also changes the default AgentDirectoryUtil behavior, and you'll never actually verify the default AgentDirectoryUtil behavior.
Or if you want to verify the two "customized" directories are actually expected. The we can have an easier unit tests for these "default" directories gen loigc.

// sessionConfigDir_
getHomeDirectory() + "/.fboss2"

// systemConfigDir_
fs::path(AgentDirectoryUtil().getConfigDirectory())
                .parent_path()
                .string()

Once again, I'm not against adding more tests but I just want to make sure we're not over-engineering here to adjust the current logics just to support unnecessary unit tests .

Please if you can, can you share what was the problem of using the default constructors back then?

[hillol-nexthop]

I landed up on a change in the dafault constructor because of which it didn't call the AgentDirUtil to get the coop dir path. It was caught in manual testing and then I realised we can cover this in UT too and so added the tests.
It's okay to ignore this change if it is going towards over-engineering direction. And anyway these set of default constructors don't go through a lot of churn so, better close the PR.