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

Tests are not included in AppMaps #351

Closed
kgilpin opened this issue Jul 18, 2024 · 3 comments · Fixed by #354
Closed

Tests are not included in AppMaps #351

kgilpin opened this issue Jul 18, 2024 · 3 comments · Fixed by #354
Assignees
Labels
enhancement New feature or request

Comments

@kgilpin
Copy link
Contributor

kgilpin commented Jul 18, 2024

Including test functions in recordings isn't helpful. Their presence is just noise, and confuses users (and Navie).

The default configuration generated by appmap-python should not include directories that primarily contain tests. Because there's no enforced standard for naming of test directories, appmap-python will exclude directories that match the regex .*test.*.

In addition, test framework integration will disable recording of test functions, in case some tests have been instrumented (e.g. because they're in a module with a name that doesn't match .*test.*). This behavior is controlled by a property named record_test_cases in appmap.yml, which defaults to false.

@kgilpin
Copy link
Contributor Author

kgilpin commented Jul 18, 2024

Current behavior

‌When no appmap.yml configuration file exists, AppMap generates a default one based on the directory structure of your project. The default configuration will automatically include any directories in your project that it identifies as Python packages. A directory is considered a package if it contains an __init__.py file.

Here is the process AppMap uses to determine which directories to include in the default configuration:

  1. Scans the root directory: It examines the root directory of your project and includes any subdirectories that it identifies as packages.
  2. Identifies packages: A directory is classified as a package if it contains an __init__.py file.
  3. Excludes certain directories automatically: It skips certain common directories that are not usually part of the implementation code, such as sys.prefix, hidden directories (those starting with .), and node_modules.

Here is a relevant code snippet from _appmap/configuration.py:

def find_top_packages(rootdir):
    """
    Scan a directory tree for packages that should appear in the
    default config file.

    Examine directories in rootdir, to see if they contains an
    __init__.py.  If it does, add it to the list of packages and don't
    scan any of its subdirectories.  If it doesn't, scan its
    subdirectories to find __init__.py.

    Some directories are automatically excluded from the search:
      * sys.prefix
      * Hidden directories (i.e. those that start with a '.')
      * node_modules

    For example, in a directory like this

        % ls -F
        LICENSE Makefile appveyor.yml docs/ src/ tests/
        MANIFEST.in README.rst blog/ setup.py tddium.yml tox.ini

    docs, src, tests, and blog will get scanned.

    Only src has a subdirectory containing an __init__.py:
    """

# Rest of the code

A default appmap.yml configuration may look like this:

name: my_project
appmap_dir: tmp/appmap
packages:
  - path: src  # Assuming src is a package containing __init__.py
  - path: tests  # Assuming tests is a package containing __init__.py

You can customize this file to add other directories or exclude ones that you don't need to be instrumented.

If you need more details on what to include or exclude, or have any specific requirements for your project setup, you can always update the appmap.yml file accordingly.

@kgilpin kgilpin added the enhancement New feature or request label Jul 18, 2024
@kgilpin
Copy link
Contributor Author

kgilpin commented Jul 18, 2024

Analysis: The find_top_packages function in _appmp/configuration.py is responsible for scanning the directory tree and identifying top-level packages that should appear in the default appmap.yml configuration. This function includes all directories that contain an __init__.py file, except for certain predefined exclusions (e.g., node_modules, hidden directories, and system prefix directories). To resolve the issue, we need to extend this exclusion list to also include typical test directories such as tests, test, and testing.

Proposed Changes:

  1. File: _appmap/configuration.py
    • Function: find_top_packages
    • Change: Modify the function to exclude typical test directories like tests, test, and testing when scanning for top-level packages to include in the default configuration.
    • Description:
      • Add tests, test, and testing to the list of excluded directories.
      • Update the logic within the excluded function to consider these directories for exclusion.

By implementing these changes, the function will no longer consider directories intended for test code, ensuring that only relevant application packages are included in the default appmap.yml configuration.

@apotterri apotterri changed the title Test directories are not included in the default-generated appmap.yml Tests are not included in recordings. Jul 20, 2024
@apotterri apotterri changed the title Tests are not included in recordings. Tests are not included in AppMaps Jul 20, 2024
@apotterri
Copy link
Contributor

[These are Navie's suggestions for how to test the change to add record_test_cases to the default config. They're all sensible, but only item 1 really needs to be done.]

Update Tests to Ensure record_test_cases Property in Default Configuration Works Correctly

Problem

The current project has been updated to include the record_test_cases property in the default configuration. Now, the tests need to be reviewed and modified to ensure this new property is correctly implemented and behaves as expected.

Analysis

To verify the inclusion and functionality of the record_test_cases property in the default configuration, the following areas need to be addressed:

  1. Testing default configuration properties to include verification for record_test_cases.
  2. Ensuring that loading configurations correctly sets the record_test_cases property.
  3. Validating that the recording behavior is influenced by the record_test_cases property appropriately.

Proposed Changes

1. Modify Tests for Default Configuration Properties:

Update the test for the default configuration to check the new record_test_cases property.

  • File: _appmap/test/test_configuration.py

  • Location: Around line 140

    Changes:

    • Update the function check_default_config to include an assertion for record_test_cases.
    def check_default_config(self, expected_name):
        assert appmap.enabled()
    
        default_config = Config.current
        assert default_config.name == expected_name
        self.check_default_packages(default_config.packages)
        assert default_config.default["appmap_dir"] == "tmp/appmap"
        assert default_config.default["record_test_cases"] == False  # Add this assertion
    

2. Add Assertions for Configuration Loading:

Ensure that the configuration loading correctly sets the record_test_cases property.

  • File: _appmap/test/test_configuration.py

  • Location: Around line 1

    Changes:

    • Update the test test_can_be_configured to add assertions related to record_test_cases.
    @pytest.mark.appmap_enabled
    def test_can_be_configured():
        """
        Test the happy path: recording is enabled, appmap.yml is found, and the YAML is valid.
        """
        assert appmap.enabled()
    
        c = Config.current
        assert c.file_present
        assert c.file_valid
        assert c.record_test_cases == False  # Add this assertion to verify the default
    

3. Update Tests for Recording Behavior:

Ensure that record_test_cases influences the recording behavior as appropriate.

  • File: _appmap/test/test_recording.py

  • Location: Around line 44

    Changes:

    • Update test_recording_clears to add an assertion related to record_test_cases.
    def test_recording_clears(self):
        from example_class import (  # pyright: ignore[reportMissingImports] pylint: disable=import-error
            ExampleClass,
        )
    
        with appmap.Recording():
            ExampleClass.static_method()
    
        # fresh recording shouldn't contain previous traces
        rec = appmap.Recording()
        with rec:
            ExampleClass.class_method()
    
        assert rec.events[0].method_id == "class_method"
        assert rec.events[0].record_test_cases == False  # Add this assertion
    
        # but it can be added to
        with rec:
            ExampleClass().instance_method()
    
        assert rec.events[2].method_id == "instance_method"
    

These changes will ensure that the record_test_cases property is correctly set and behaves as expected in different scenarios. This will cover default configuration, configuration loading, and recording behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants