Run acceptance tests in both github and gitlab #1245
Conversation
| func setGitProvider() (gitproviders.GitProviderName, string, string) { | ||
| gitlab := "gitlab" | ||
| github := "github" | ||
| gitProvider := os.Getenv("GIT_PROVIDER") |
There was a problem hiding this comment.
Please use os.LookupEnv(key) instead, it will help in knowing if this env is set or not. Which is relevant when we run tests locally. In that case and even for CI, you could add some logic saying we need to set this environment variable for tests to work properly(or something similar), otherwise running tests locally will fail, and we would need to debug just to find out that missing this env was the actual problem. If possible I would move this logic to here https://github.com/weaveworks/weave-gitops/blob/main/test/acceptance/test/testsuite_common_test.go#L52 as this already does what I'm suggesting. And it helps to have one single place for the same logic patterns.
There was a problem hiding this comment.
But the tests won't fail even if the env is not set. The function returns github by default even if the env is missing. Si the tests would run on github by default. The only change this function brings is if we want to run all the github tests in gitlab repos. I don't want this variable set as a requirement because I avoided using this env for specific gitlab tests.
There was a problem hiding this comment.
That makes sense. I would argue then that we should have some doc explaining this behavior and also how to run all this cases, like:
- How to run github tests in gitlab
- How to run gitlab tests
- How to run github tests
BTW if you need to have a different value for specific variable in one of the tests you can add it to that specific test only. Like here for instance https://github.com/weaveworks/weave-gitops/blob/main/test/integration/server/remove_test.go#L254. This AutoMerge variable is not needed in all cases only on that specific one. If you ever need to use something specific in more than one test, and you want to avoid writting it twice , you could add a new Context and set that variable's value in a BeforeEach within that context. Like this for instance https://github.com/weaveworks/weave-gitops/blob/main/test/integration/server/remove_test.go#L223. gitlabGroup was written once but it runs for each test within that context.
There was a problem hiding this comment.
I can definitely update the ReadME file for clarification
There was a problem hiding this comment.
I'll update the ReadME in a follow up PR and try to see if setting git-provider variables at BeforeSuite works. For now, I'm merging this PR.
There was a problem hiding this comment.
NVM, I updated the readme and set the variables at suite level.
test/acceptance/test/utils.go
Outdated
| @@ -845,3 +846,18 @@ func getGitProvider(org string, repo string, providerName gitproviders.GitProvid | |||
|
|
|||
| return gitProvider, orgRef, err | |||
| } | |||
|
|
|||
| func setGitProvider() (gitproviders.GitProviderName, string, string) { | |||
There was a problem hiding this comment.
Let's use this logic only once here https://github.com/weaveworks/weave-gitops/blob/main/test/acceptance/test/testsuite_common_test.go#L52 instead of calling it multiple time in each BeforeEach. If you want to keep this function and call it once in BeforeSuite it's fine I would just suggest changing the function name to something more align to its functionality, maybe something like getGitProviderInfo,getGitProviderData or something similar. Usually methods with set in their name are meant to receive parameters and do not return values.
| @@ -71,7 +74,7 @@ runs: | |||
| run: | | |||
| chmod +x bin/gitops | |||
| ls -la bin | |||
| - name: Download dependecies | |||
| - name: Download dependencies | |||
* Attempt to fix env vars by using double quotes * Print out git provider value * Print out git provider value * Removed extra dollar simbol * Removed debug code
Closes: #1251
What changed?
GIT_PROVIDERfor test runsrun testjob to run 2 test cases on parallelcreate a pr for app deletionWhy?
--auto-mergeflag is skippedHow did you test it?