RHOAIENG-6869: Support running tests based on values given to the e2e tests #266
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| username: "{{ IMAGE_REGISTRY_USERNAME }}" | ||
| password: "{{ IMAGE_REGISTRY_PASSWORD }}" |
There was a problem hiding this comment.
Added quotes to some of the template files as it was breaking the go yaml parser. I think this shouldn't break the shell tests but I have not tested it
| ifdef GIT_SELF_SIGNED_CERT | ||
| oc create configmap git-self-signed-cert --from-file=ca-bundle.crt=${GIT_SELF_SIGNED_CERT} | ||
| endif | ||
| ifdef S3_SELF_SIGNED_CERT | ||
| oc create configmap s3-self-signed-cert --from-file=ca-bundle.crt=${S3_SELF_SIGNED_CERT} | ||
| endif |
There was a problem hiding this comment.
I still need to re-add this part as part of the go testing setup stage
|
I requested review but still need to update docs and the change I mentioned above. Waiting to do that as I want to get an opinion on the work so far. |
jackdelahunt
changed the title
|
@jackdelahunt: This pull request references RHOAIENG-6869 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
jackdelahunt
force-pushed
the
configure-e2e-tests
branch
from
bf30ab2
to
212f0e6
Compare
|
@jackdelahunt: This pull request references RHOAIENG-6869 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
For example, if you are only trying to run the main MLOps pipeline it is not needed to give the values for gitops. If all of the values in the config for gitops are empty then those tests are not run.
The config.json is structured to have all non-specific values at the root (NAMESPACE etc.) and then all optional values which will be the conditions for tests in their own sub object, here labelled s3-fetch and git-fetch as they are the two options for fetching.
I'm not very enthusiastic about the idea of having a configuration section filled to have a second implicit meaning of "always run the test(s) that use this section".
For example, just because I've got values specified in the "gitops" section, doesn't mean that I always want to run the gitops tests, right?
Is it still possible to run specific tests (or sets or tests) without taking stuff out of the config file?
Also, regarding the config file, there are two surprising (IMHO 🙂) choices:
- JSON rather than YAML (or something else)
- CAPS_LOCK for the keys
Maybe just passing the types of tests you want to run through a env variable. Or define what is run in the config file aswell, not exactly sure how that would work though
Go has built in support for json and yaml so I just picked one
This was a carry over from when they were passed and env variables. But yeah they can change |
Using tags would be one possibility: https://mickey.dev/posts/go-build-tags-testing/ Another would be to have some sort of |
I have used these before and they were a hassle to deal with but maybe if I was to look into again it might work well here. Seperate test files have a build falg behind them. How would you pass this to the compiler from make, in a way that isn't a pain
This is for sure a thing I was thinking about, in the config types I even have these alerady just that they are infered from the file instead of being set by it. So I think that could be okay. Something like this I guess you are saying and have this on each sub object in the config file right? |
test/e2e-tests/support/config.go
Outdated
| GitFetchEnabled bool | ||
| S3FetchEnabled bool | ||
| GitOpsEnabled bool |
There was a problem hiding this comment.
@grdryn Just about what we were saying, moving these to the GitFetchConfig etc. could work
We don't necessarily need to require the tests be run by make, right? I'd keep the default invocation that CI would use to run all tests in the Makefile, but there's a bunch of stuff that |
Yeah I probably agree I just thought we were trying to do everything through the Makefile. About using build flags, I feel like relying on compiler flags is something we wouldn't want but now that we are saying we just say calling |
|
@jackdelahunt: This pull request references RHOAIENG-6869 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jackdelahunt: This pull request references RHOAIENG-6869 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@jackdelahunt what do you mean by that? This would be just passing the tags variable content or similar to the go test command. |
There was a problem hiding this comment.
A couple of things to think/discuss:
- Do we need to have this done using a separate config file? Why not having simple switches for meaningful groups of tests (s3, git, gitops...) as Makefile variables? Or we can utilize naming conventions and then use
go test -run regexpto include just the needed tests. I am not aware of any functionality/library which would enable us tagging each test function like in Java using annotations. The closes to that are Kubernetes/Kubebuilder markers or codnect/markers but these are just for static analysis/code generation apparently. - I would prefer to provide test parameters using command line rather than using a file. Maybe we could have default Makefile variables defined in the Makefile and then do the validation using Go as Makefile
ifndef ... endifcan be quite repetitive and combining it withenabledflags may make it even worse. It is not that expensive to validate parameters later in Go and not earlier in the Makefile IMHO. This also pairs well with the first idea of having these enabled switches as normal Makefile variables. We would just validate the corresponding params only if the switch is NOT false, otherwise the params tied to that flag won't be validated. Tests would then be checking the enabled flags as they do now.
I guess this could work.. I was thinking about this as then we would also need to do the validation in make but as from what you said you are saying to pass all the variables in, run setup based on other variables set in make like
My idea with this was to then be able to save a configuration or setup multiple to be run. For example keep the same config but change the bucket and git repo and use self signed certs. But maybe this just ins't a use case. Another reason for using a cofig is that it is more clear what variables are used where. I can see a case happening where a person is confused but trying to give values for fields that won't be used. This could probably be helped by good docks and clear naming of course aswell. |
Validation can be done in Go later. I think it would be more clear to validate everything in Go rather than in Makefile with complicated branching. If we had an expensive setup done before running Go, then it would make sense to have it in Makefile to fail fast but if we delegate this to Go, it won't be seconds/minutes later that we find out some parameter is not filled.
Makes sense then. However, I would like to retain the possibility to pass variables from command line, i.e. not having to use the config file every time. But that may be just my preference. It's true that for CI it may be possible to have a predefined config file and then we can just reference it as an argument/using a Makefile variable. This way we won't need to run sed every time to overwrite the default config, for example. |
Okay I will work on getting that done then 👍
That's true but if every input is thorugh a env variable then we can just load a |
|
Also just as another question, when we are selecting which tests to run (git fetch, gitops etc.) which way is nicer to have those flags.
|
I would go with option 1. Option 2 is a bit more elegant imo, but having one flag per test-suite is easier for the user, so you only need to enable/disable the flags related to the test-suites you want to run. |
|
@MarianMacik just updated the PR to load the environment instead of a config file, here are my thoughts so far after using it a bit.
|
I didn't say it would be through env variables, but through the Makefile variables. It's true that then these would need to be translated to env vars for go test or to cobra flags.
Why are more env variables required when moving setup to go? These variables/params will need to be somewhere anyway because the test suite simply needs this data.
I understand, though we don't need to pass every variable every time if we configure some defaults in the test suite.
In the previous project Kogito operator we had Makefile variables which were then translated using Makefile itself to flags with prefix Also, what did you mean by being it messy when passing from the command line and not messy when using a config file? Did you mean that config file is automatically mapped to a Go struct without having to query environment variable one by one? For command line arguments this can be solved by using Cobra, which will automatically parse and map parameters to respective Go variables, for example. I am not against the config file, just trying to already think about how it will be used by users. If it is run locally, then a user will override the config file with custom values or use a custom config file. For CI we would either need to override the config file using |
Sorry, I didn't actually mean there are more because of go just that I added ones that were missing before that make it much more configureable compared to before
This is true but I think many of these defaults are going to need to be set to whatever the user needs more often then not. So much of the options like names, credentials and build tags are specific to the person running it meaning the default won't cover much use cases. Of course setting them to something the pr-check will use means it will be easier for that atleast and you still have some defaults there
I think I understand whats happening but not sure but I do get the next section.
Would
I meant two things:
There are defiantly pros and cons, I am not sure if a config file is the best idea either. Right now I am just feeling the current solution of passing env vars is not my favouite but maybe some of the others mentioned could work instead of either.
Yes that was my idea, for the CI using sed or a custom config can both work for it so either way is fine. For local my idea was, when you clone the repo you get the config template and then copy it (because of gitconfig and maybe leaking secrets) to create your own with all your needed fields filled in. |
go test can work with Cobra flags, the same approach was used with the Kogito operator I posted.
What did you mean by "gitconfig" here? OK, I think we can go with config file then. We just need a way how to reference a custom config file, either through environment variables or using a Cobra flag (maybe too complex for one parameter, it would also require to use TestMain instead of normal test approach). |
Sorry wrong one meant .gitignore. It was just a comment on the fact that if we want to have an up to date config example we should track that but of course we dont want people using that in case they accidentally commit secrets.
Yep that makes sense, it also looks almost the exact same as the init funtion just with out the testing info passed in. I guess it is better for that but I don't know why they both exist. If I dug deeper maybe the scope at which they are called once for different... https://github.com/opendatahub-io/ai-edge/blob/main/test/e2e-tests/tests/pipelines_test.go#L61-L63 |
jackdelahunt
force-pushed
the
configure-e2e-tests
branch
from
835e0dc
to
ee991f0
Compare
Description
This PR aims to make the go e2e test more configurable and more usable for cases in which you want to test but don't have all the requirements for the different types of tests.
Using a
config.jsonthe values are passed to the e2e tests. Based on the this config the e2e tests may or may not run certain tests, this is set from theenabledfield in each sub object in the config.{ "namespace": "", "image_registry_username": "", "image_registry_password": "", "target_image_tags": [], "git_fetch": { "enabled": false, "container_file_repo": "", "container_file_revision": "", "container_relative_path": "", "model_repo": "", "model_relative_path": "", "model_revision": "", "model_dir": "", "self_signed_cert": "" }, "s3_fetch": { "enabled": false, "aws_secret": "", "aws_access": "", "region": "", "endpoint": "", "bucket_name": "", "self_signed_cert": "" }, "gitops": { "enabled": false, "token": "", "username": "", "repo": "", "api_server": "", "branch": "" } }For example, if you are only trying to run the main MLOps pipeline it is not needed to give the values for gitops. By setting the
enabledfield you can set the gitops pipeline not to run.The
config.jsonis structured to have all non-specific values at the root (Namespace etc.) and then all optional values for tests in their own sub object, here labelleds3_fetchandgit_fetchas they are the two options for fetching.How Has This Been Tested?
make go-testMerge criteria: