Test refactor

[mknorps]

As we advance in the complexity of objects representing the FD domain, our test code also grows in complexity.
It takes notably more time to change the test code base when changing the basic objects that are being tested.

We should:

• rethink the structure of tests, as discussed with the team, to test one feature at a time (for example change in Location object should not propagate to all tests using UndeclaredDependencies)
• gather common helper functions in utils.py and refactor tests cases to use them
• Use more fixtures not to repeat test cases between tests files and/or generate test cases automatically
• Consider using objects with reasonable defaults for pytest.param inputs (test in `compare_imports_to_dependencies) have now 5 variables, for which most is just an empty list
• (may be a separate issue) Consider using hypothesis for property-based testing

[Nour-Mws]

I concur. Changing the tests when adding new functionality is becoming increasingly complex and thus error-prone. This might or might not be due to a design problem in these tests. Your list is a very nice way to start thinking through this.

[jherland]

Relevant comments from #193:

@mknorps:

Nit/Improvement idea:
The number of parameters for this test becomes impossible to read. A lot of them are empty and we just add one or two fields. I think it matured enough even to be added in this PR.
Proposition outline:

@dataclass
class CompareI2D:
    imports: List[ParsedImports] = field(default_factory=list)
    dependencies: List[DeclaredDependency] = field(default_factory=list)
    ignore_unused: List[UnusedDependency] = field(default_factory=list)
    ignore_undeclared: List[UndeclaredDeps] = field(default_factory=list)
    expected: Tuple[Dict[str, Package], List[UndeclaredDependency], List[UnusedDependency]]

and then for example:

        pytest.param(
            imports_factory("invalid_import"),
            [],
            [],
            ["invalid_import"],
            ({}, [], []),
            id="one_ignored_undeclared_dep__not_reported_as_undeclared",
        ),

should become:

        pytest.param( 
                {
                 "imports": imports_fac (tory("invalid_import"),
                 "ignore_undeclare": ["invalid_import"],
                 "expected": ({}, [], [])
                },
            id="one_ignored_undeclared_dep__not_reported_as_undeclared",
        ),

and create CompareI2D object in the test function.

@jherland:

I agree that the tests are becoming unreadable, and I think the rewrite you suggest is good improvement.

That said, I want to go even further (in a future PR): I am planning a rewrite of compare_imports_to_dependencies() to split it into three: Currently it is one function that calculates three different things (resolved_deps, undeclared_deps, and unused_deps), and then returns those three things, but AFAICS there is no complex interplay in these calculations, and it should be straighforward to split into three separate functions: resolve_dependencies(), calculate_undeclared(), and calculate_unused().

With that refactoring, it is natural to revamp the tests as well, and we will want to test each function in isolation. Testing one function at a time will naturally limit the amount of data that we pass in and expect out in each test case.

So in short, although I agree with the rewrite you suggest, it might end up not surviving a soon-to-come (bigger) rewrite... thinking

[jherland]

FWIW, Maria's proposal in the above comment led to commit b0fbc8d in #202.

I think this kind of refactoring is very valuable, as it makes it much easier to read a single test vector in isolation and understand its objective. 🎉

[jherland]

This issue was discussed in today's status meeting, and a couple of points from the meeting notes are worth repeating here:

• Testing with hypothesis is very valuable, but applying it at the CLI level is too slow. Write hypothesis tests directly against the Analysis object instead.
• Focus more on testing with the Analysis object. It should be our main test “surface” esp. if FD ends up being used as a library.
• Rephrase CLI tests as Settings+Analysis tests. Not having to go through a subprocess will speed these tests up considerably.
• (for later:) Consider the overlap of "sample projects" defined in tests/conftest.py vs. the projects in tests/sample_projects.