Conversation
MDrakos
force-pushed
the
CP-1054-follow-up
branch
from
9ef79cb to
0aa1695
Compare
| func getAPIUpdateURL(cfg Configurable, path string) string { | ||
| var apiUpdateURL string | ||
|
|
||
| envUrl := os.Getenv("_TEST_UPDATE_URL") |
There was a problem hiding this comment.
This doesn't feel like something we do. We usually have a constant that contains an override env var we can use for testing. What do you think about doing that here?
| cp = ts.SpawnWithOpts( | ||
| e2e.OptArgs("update"), | ||
| e2e.OptAppendEnv(suite.env(false, false)...), | ||
| e2e.OptAppendEnv("VERBOSE=true"), |
There was a problem hiding this comment.
Note: we can use the "-v" parameter to enable verbose logging. I fixed it late last year or earlier this year.
| e2e.OptAppendEnv(suite.env(false, false)...), | ||
| e2e.OptAppendEnv("VERBOSE=true"), | ||
| ) | ||
| cp.ExpectExitCode(11) // Expect failure due to DNS resolution of fake host |
There was a problem hiding this comment.
If an error will be logged, then we also need to call ts.IgnoreLogErrors().
The nightly integration tests scan the logs for errors and fail tests if errors are found unless we expect to have them.
MDrakos
force-pushed
the
CP-1054-follow-up
branch
from
a69ff22 to
a5a7343
Compare
mitchell-as
left a comment
mitchell-as
left a comment
There was a problem hiding this comment.
I saw another instance of a magic env var that we can either address now or file a follow up for. It's up to you.
| @@ -87,7 +87,7 @@ func (u *Checker) infoURL(tag, desiredVersion, branchName, platform, arch string | |||
| infoURL string | |||
|
|
|||
| envUrl = os.Getenv("_TEST_UPDATE_INFO_URL") | |||
There was a problem hiding this comment.
Sorry, I just saw this. Can we change this env var to use a constant too? If you don't want to, then please file a follow-up ticket.
There was a problem hiding this comment.
Thank you for catching that! 🙏
MDrakos
force-pushed
the
CP-1054-follow-up
branch
from
a5a7343 to
dd35ebf
Compare
2 participants
In the previous PR I mistakenly used the config value to override the update info endpoint. I still think we need that, however the story calls for this config value to override the update endpoint. I've added another config value specifically for the update info endpoint and repurposed the original one for the indented use case.
Integration test failures appear to be unrelated to this PR as the same failures are happening on the nightly tests.