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

Refactor smoke_test.py test fixtures #585

Merged

Conversation

jcar87
Copy link
Contributor

@jcar87 jcar87 commented Nov 7, 2023

Description

Refactor fixtures such that each test uses its own cmake source and build directory, rather than reusing them across tests. This helps those tests that need to mutate the source directory, and also prevents pytest-ordering issues or workarounds by never reusing a build directory.

Motivation

The session scope basic_setup and the tmpdirs fixture, on top of setting up a CONAN_HOME that is custom to the entire test session, set the current working directory of all tests to be the same. The function-scope fixtures chdir_build and chdir_build_multi then simply chdir into either build directory. Additionally, the conan provider is copied to this working directory alongside the sources. This has some problems:

  • The structure of the basic cmake project for testing is common to all tests. This would be fine, however, some tests mutate the contents of this "source" directory - so tests run after would be configuring an entirely different project. If it has worked so far, it's by coincidence.
  • The build directory is reused between tests - this has resulted in tests that need a clean state to either pass --fresh to the cmake invocation (to ignore anything that was there beforehand), or to have class-scope fixtures to delete the contents of the build directory, if it exists. One test, however, relies on an already initialized build directory to exist.

Detailed changes

  • Separate concerns by creating two new fixtures that are global to the session:
    • conan_home_dir: sets the CONAN_HOME environment variable
    • setup_conan_home: exports the relevant recipes
  • Use pytest's built-in tmp_path_factory or tmp_path fixtures, rather than doing this manually via tempfile - pytest handles the automatic deletion of past temporary directories and there's no need to do that ourselves.
  • Create a basic_cmake_project test-level fixture:
    • Create a per-test temporary working directory
    • Call setup_cmake_workdir() (new function) to copy the contents of the basic cmake project for testing
    • Create a build folder, and chdir into it
    • This is unique to each test that uses this fixture, so neither the source folder nor the build folder are reused.
  • For tests that need a different setup, they can opt to not use the test-level fixture, and handle this differently.
  • Use a global variable for the path to the conan_provider.cmake file - It's static and is not mutated by any of the tests, and it doesn't need to be copied.

Refactors:

The above changes enable the following:

  • The TestBasic class assumes a re-used build directory, and a separate build-multi directory - this is now handled in a new setup_basic_test_workdir() class-level fixture. This is done to enable minimal changes to the already existing tests in this class
  • Refactor all find module tests into a single class - there's no no longer a need to be in different classes each with a different class-scope fixture - since each test uses basic_cmake_project, each can mutate the contents of the source directory by copying the extra files from the resources directory.
  • The TestSubdir uses a custom setup (different resources) - so handle it with a custom fixture in that class
  • Refactor TestSubdir to support Windows - there's no reason why it wasn't supported
  • Remove all workarounds passing --fresh to CMake - they're all fresh now, because the build directory is empty when using the basic_cmake_project fixture
  • Remove workarounds clearing the contents of the build directory in a fixture - no longer needed, since it's guaranteed to be empty when using basic_cmake_project.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking solid

Copy link
Contributor

@juansblanco juansblanco left a comment

Choose a reason for hiding this comment

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

Looks good.
We could group all the 'module' resources in a parent folder since the tests that use them are all in the same class now.

@jcar87 jcar87 force-pushed the lcc/feature/refactor-smoke-test-fixtures branch from ef8ebda to a4882aa Compare November 8, 2023 13:33
@jcar87 jcar87 merged commit 2bff09e into conan-io:develop2 Nov 9, 2023
5 checks passed
@jcar87 jcar87 deleted the lcc/feature/refactor-smoke-test-fixtures branch November 9, 2023 10:11
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.

3 participants