Skip to content
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

Simplify Test_LDAP #505

Merged
merged 4 commits into from
Sep 21, 2022
Merged

Simplify Test_LDAP #505

merged 4 commits into from
Sep 21, 2022

Conversation

dataclouder
Copy link
Contributor

@dataclouder dataclouder commented Sep 17, 2022

  • Remove VM and network creation from LDAP test
  • Replace ephemeral VM with permanent LDAP server in the testing VCD
  • Remove functions that were only used to create the VM for this test

This is similar to the change of TestAccVcdOrgGroup in PR-909

The reason for this change is that what we are testing within TestLDAP is quite fast, but the preparation takes a long time, so much that we added a tag skipLong to avoid such test in given circumstances. Moreover, creating a LDAP server (which requires routing to the VM) would fail for environmental reasons that add nothing to our testing experience.
The change requires a pre-set LDAP server, which is created only once per each VCD in a separate –automated– task, thus allowing us to test the intended feature without waiting for the server to be up.

* Remove VM and network creation from LDAP test
* Replace ephemeral VM with permanent LDAP server in the testing VCD
* Remove functions that were only used to create the VM for this test

Signed-off-by: Giuseppe Maxia <gmaxia@vmware.com>
Signed-off-by: Giuseppe Maxia <gmaxia@vmware.com>
@dataclouder dataclouder marked this pull request as ready for review September 17, 2022 09:08
@adambarreiro
Copy link
Collaborator

First review, all good imho! I'll run the tests and then approve.

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One ask only: for our own reference please add the explanation "why" we're doing this change in PR description. Thanks!

Copy link
Collaborator

@adambarreiro adambarreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the tests with the user tag and all passed OK. They're much faster now that they don't spin-up the LDAP container. Thanks!

Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests worked great. The only query I have is to remove Misc.LdapContainer variable from TestConfig struct and from sample_govcd_test_config.yml

Giuseppe Maxia added 2 commits September 20, 2022 10:04
Signed-off-by: Giuseppe Maxia <gmaxia@vmware.com>
Signed-off-by: Giuseppe Maxia <gmaxia@vmware.com>
@dataclouder dataclouder merged commit 4c4d84b into vmware:main Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants