-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix failing integration test #2816
Conversation
MDrakos
commented
Oct 13, 2023
•
edited by github-actions
bot
Loading
edited by github-actions
bot
DX-2246 Nightly failure: TestInstallScriptsIntegrationTestSuite/TestInstall/install-prbranch-with-version |
Note that we will not run into this issue with our regular integration tests because they copy binaries from the build directory and execute those directly. The issue surfaced in this test because we are running |
// The home dir is set already for the test session, but we need to | ||
// set it for the test so helper functions can find it. | ||
suite.T().Setenv(constants.HomeEnvVarName, ts.Dirs.HomeDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find, but we should probably do this as part of e2e.New()
. Otherwise we'll likely keep running into this.
installPath, err := installation.InstallPathForBranch(constants.BranchName) | ||
suite.NoError(err) | ||
|
||
binPath := filepath.Join(installPath, "bin") | ||
statePath := filepath.Join(binPath, "state"+osutils.ExeExt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e2e should handle this, and if it's not it'll need to be fixed there. You should be able to use ts.Exe
.
But that is too much here. This shell should have no awareness of the CI State Tool. It sounds like we're bleeding the state tool PATH from the CI environment into our test environment. We should focus on fixing that because there's no way we'll always remember this caveat.
Co-authored-by: Nathan Rijksen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but as we said this is an interim workaround. I'd like to see comments calling this out as a temp workaround with a link to the jira story so we don't lose track of the core issue.
For some reason I thought v42 was already out so changed the base to v43... I've now changed it back to v42 🤦♂️ |