chore: parameterize SubCommandTest with configLevel #239
Conversation
| func (suite *SubcommandTestSuite) TearDownTest() { | ||
| suite.Require().NoError(os.Unsetenv("ADBC_DRIVER_PATH")) | ||
| } |
There was a problem hiding this comment.
why remove this? If we're not going to remove this env var after each test, then we should move the os.Setenv to SetupSuite and add Unsetenv to TearDownSuite
There was a problem hiding this comment.
This got split out into platform specific TearDownTest methods (subcommand_unix_test.go, subcommand_windows_test.go)
| // Clean up the registry and filesystem after each test | ||
| _, user := os.LookupEnv("DBC_TEST_LEVEL_USER") | ||
| _, system := os.LookupEnv("DBC_TEST_LEVEL_SYSTEM") |
There was a problem hiding this comment.
what happens if both are set?
There was a problem hiding this comment.
If both are set, cleanup for both should run. Does that sound right?
config/config.go
Outdated
| func GetLocation(lvl ConfigLevel) string { | ||
| return lvl.configLocation() | ||
| } |
There was a problem hiding this comment.
why not just export lvl.configLocation instead?
There was a problem hiding this comment.
The last time you and I were looking at doing this, I think you mentioned not wanting to export lvl.configLocation so I exported a wrapper here instead. I'll remove the wrapper since I think it's fine just to export lvl.configLocation.
There was a problem hiding this comment.
Done in b7a3289. level.ConfigLocation is now exported.
| return fmt.Errorf("error removing manifest %s: %w", manifest, err) | ||
| } | ||
|
|
||
| // TODO: Remove this when the driver managers are fixed (>=1.8.1). |
There was a problem hiding this comment.
is there an issue on arrow-adbc to track this?
There was a problem hiding this comment.
This was fixed in apache/arrow-adbc@526dd93. We can file an issue here on dbc to do something about it.
cmd/dbc/subcommand_test.go
Outdated
|
|
||
| func (suite *SubcommandTestSuite) SetupTest() { | ||
| suite.tempdir = suite.T().TempDir() | ||
| suite.Require().NoError(os.Setenv("ADBC_DRIVER_PATH", suite.tempdir)) |
There was a problem hiding this comment.
actually, we could save ourselves some trouble and just use suite.T().Setenv, let's do that
There was a problem hiding this comment.
Good call. I didn't know about that.
I realized we can make the change more comprehensively too so I did that in 643f102.
| # User and System level tests | ||
| - name: Run Tests (User) | ||
| run: go test -v ./... | ||
| env: | ||
| DBC_TEST_LEVEL_USER: 1 |
There was a problem hiding this comment.
do we need one of these with DBC_TEST_LEVEL_ENV: 1?
I also really don't like this personally, but can't think of a better option other than maybe using TestMain and adding explicit command line args that we look for instead? meh, I can live with this for now.
There was a problem hiding this comment.
We don't, just running the tests normally (which we do above) tests DBC_TEST_LEVEL_ENV.
This was my third attempt at this and I dislike this approach the least. I can live with it too :)
|
Thanks for the review @zeroshade. Ready for a re-review. |
2 participants
Extends the SubCommandTest test suite to be parameterized by configLevel so the subcommand tests can be run with
--levelset to user or system by running the test suite withDBC_TEST_LEVEL_USER=1orDBC_TEST_LEVEL_SYSTEM=1respectively.This is not enabled by default on developer machines but is enabled in CI. To opt in this behavior, tests need to be written to use
suite.configLevelto control their config level and tests can opt out either by not doing this or by doing this but adding appropriate Skips.To demo what we can do now, this PR also includes a fix for #215. We could have fixed the issue by testing in a tempdir but with the change in this PR we can actually test the bug that was reported. Doing this also uncovered some edge cases in the symlink behavior so this was time well spent.
Also fixes a minor undiscovered bug in removing manifest-only drivers: e69744f.
Supersedes #238
Closes #210
Closes #215