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

VPN-5560 - Do not run auth_tests on PRs #8245

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Conversation

brizental
Copy link
Contributor

@brizental brizental commented Oct 9, 2023

No description provided.

@brizental brizental changed the title VPN-5560 - Do not run auth_tests alongside unit tests and do not run them on PRs VPN-5560 - Do not run auth_tests on PRs Oct 10, 2023
@brizental brizental marked this pull request as ready for review October 10, 2023 08:45
@brizental
Copy link
Contributor Author

Someone that has admin right on this repository needs to add the auth_tests job to the checks run on merge to main and release, I think. cc @lesleyjanenorton, @oskirby

@brizental
Copy link
Contributor Author

Someone that has admin right on this repository needs to add the auth_tests job to the checks run on merge to main and release, I think.

Actually doesn't look like there is anything that needs to be changed 🤷‍♀️

Copy link
Collaborator

@oskirby oskirby left a comment

Choose a reason for hiding this comment

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

I don't think this is going to achieve the desired result, the list of tests in CMake is generated by the add_test() function and get run by ctest. However, in the tests/auth_tests/CMakeLists.txt we are still adding the tests, so they will are still being run on the Unit test job.

What we really need is a way to split off the auth tests so they are not included by ctest unless we explicitly build for them. This could be done by adding something like this to the top-level CMakeLists.txt:

option(BUILD_E2E_TESTS "Build end-to-end tests" FALSE)
if(BUILD_E2E_TESTS)
  add_subdirectory(tests/auth_tests EXCLUDE_FROM_ALL)
endif()

@oskirby
Copy link
Collaborator

oskirby commented Oct 10, 2023

I don't think this is going to achieve the desired result, the list of tests in CMake is generated by the add_test() function and get run by ctest. However, in the tests/auth_tests/CMakeLists.txt we are still adding the tests, so they will are still being run on the Unit test job.

What we really need is a way to split off the auth tests so they are not included by ctest unless we explicitly build for them. This could be done by adding something like this to the top-level CMakeLists.txt:

option(BUILD_E2E_TESTS "Build end-to-end tests" FALSE)
if(BUILD_E2E_TESTS)
  add_subdirectory(tests/auth_tests EXCLUDE_FROM_ALL)
endif()

Oh wait! I am dumb, I see how you got around it, you are using the --test-dir argument on ctest to run a subset of tests.

@brizental
Copy link
Contributor Author

I don't think this is going to achieve the desired result, the list of tests in CMake is generated by the add_test() function and get run by ctest. However, in the tests/auth_tests/CMakeLists.txt we are still adding the tests, so they will are still being run on the Unit test job.

That was actually on purpose. I started out and was going to do something similar to what you proposed, but then I decided to keep this minimal and actually just changed the CI job instead.

@brizental brizental merged commit 3b70bfb into main Oct 10, 2023
83 checks passed
@brizental brizental deleted the 5560-goodbye-auth-tests branch October 10, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants