pytest-dependency icon indicating copy to clipboard operation
pytest-dependency copied to clipboard

Support for reordering tests based on dependencies

Open DevilXD opened this issue 4 years ago • 33 comments

This one is based off #43, and thus includes it's changes here as well. Assuming that one would be merged, this one could be merged right after - I did it this way to properly cleanup the inconsistencies across the entire file.

The added hook function will reorder the test items based off the dependencies specified, taking into account scopes as well. There is a warning emitted for every dependency that's specified, but is not found during collection - this comes at a cost of having to process the items twice, but I think it's helpful enough to justify it.

This closes #20, and technically closes #43 too.

EDIT: Since it clearly seems that this will never be merged, please consider using pytest-order, which provides the same functionality (and more), allowing you to reorder the execution of tests, making them dependent on each other.

DevilXD avatar Jun 10 '20 15:06 DevilXD

Wanted to add that this new functionality could be gated behind a command line option too, if needed.

DevilXD avatar Jun 10 '20 17:06 DevilXD

@DevilXD, for some reason this fix reverses the order of all the test cases, so I have to use reverse package to make it correct again.

6lor avatar Jun 11 '20 17:06 6lor

I'm... not sure why this might be. This is the order they execute for me, which appears to be fine: https://i.imgur.com/2cA2cMJ.png

This change should not alter any tests that have no dependencies, and those with missing dependencies could end up being pushed lower down the list, because it will be trying to add tests with existing dependencies first, before realizing that it's a cycle and no other dependencies exist, and then adding the rest in the order they remain.

Overall, outside of altering the order of the tests in which they execute, there should be no downsides to this, other than some additional processing required for reordering of course.

DevilXD avatar Jun 11 '20 20:06 DevilXD

Just wanted to note here that since I know what this problem boils down to now (Topologically Sorting a Directed Acyclic Graph), I could try working on this more so that the resulting list is guaranteed to be stable - that is, the items aren't reordered unless absolutely necessary. The current solution is to loop through all items and only add them to the final list, if all of their dependencies have been added there already as well, which (assuming there's no cycles or missing dependencies) guarantees the correct ordering. Not sure on it's stability, but there's lots of room for improvement in terms of speed for sure.

I'm marking this as a draft for the time being.

DevilXD avatar Jun 15 '20 08:06 DevilXD

Alright, so I think I've managed to make this one much better than it was initially. Key changes (compared to the previous version):

• previous version had a theoretical desync bug between registering a dependency and adding it to the final list, that could occur in rare circumstances - most of the previous code has been replaced, so it's not the case anymore • fixed a tiny issue with an undefined variable, a leftover from making _remove_parametrization a function - it was in the "RuntimeError else" part that was never supposed to run, so I didn't catch it the first time • reverted the commit that changed the output ordering of two tests - the new sorting method is stable, so it won't reorder anything if it's not required • improved the sorting method - it now always iterates twice over all test items: once to build a dictionary of item names to indexes, and the second time to adjust indexes so that dependencies fall before the item that requires them. The previous version did one pass to gather names, and then multiple passes over a deque of items, recycling back anything that didn't have its dependencies in the final list yet, which meant at least one loop per "nesting level" of those (where an item depends on an item, that in turn depends on another one too) - it's now always two passes only. This means faster collection as well. • a warning is still emitted during sorting, if there's a dependency detected that cannot be found • both, missing dependencies and cyclic dependencies, will not cause this code to hang on the collection phase • important note: in case where cyclic dependencies exist, this will try to move the former test closer to the latter one, but of course this will still cause both of them to be skipped

Tests pass on my side, in the same order as on the picture I sent previously (https://i.imgur.com/2cA2cMJ.png), and this thing works great on another repo of mine with 50+ tests and ~4 nesting levels of dependencies, so I'd say that it's ready to go.

DevilXD avatar Jun 15 '20 21:06 DevilXD

While playing with this in my test file, I discovered a bug that may still fail to reorder dependencies in some cases. Because I currently don't have an easy fix for it, I'm gonna go back to the drawing board for the time being, and probably add some proper tests for this thing too.

DevilXD avatar Jun 17 '20 07:06 DevilXD

Turns out that working with indexes like that, while cool in theory, turned out to be quite problematic in practice. I couldn't manage to keep a stable sort order with that approach, so I've reverted to the previous one with deque and recycling items - this time tho, the items for which the dependencies weren't found yet, will be rechecked with each final item added, which ensures stability. Additionally, this means that items with missing or circular dependencies will always end up executing at the very end.

There are 3 tests gating this functionality now, with tests for a standard sorting case, cycles, and nested dependencies. I should probably add one for missing dependency as well. Overall, even tho it's probably not the most efficient sorting method, it looks like it's finally ready for review.

DevilXD avatar Jun 17 '20 17:06 DevilXD

@RKrahl Could this be looked at? The algorithm has O(n) average time complexity, with O(n²) at the absolute worst case scenario. I'm assuming no one will reverse-order their tests with reverse-dependencies though, so that shouldn't matter too much. The sort can be made slightly faster overall by sacrificing it's stability (recycle missing deps items to the end instead of beginning for rechecking), but it's still gonna have to be O(n) average and O(n²) worst case.

DevilXD avatar Jun 29 '20 08:06 DevilXD

Any hope of getting this merged? This feature is highly desirable, especially because many blogs and such mention this package also handles ordering...

james-powis avatar Sep 24 '20 16:09 james-powis

@RKrahl Could we have it pushed forward please? Dependency reordering is something really missing.

lukjak avatar Nov 11 '20 18:11 lukjak

For what It is worth, I managed to obtain ordering by taking advantage of the alphabetical execution order for all test functions in an individual class via method naming convention and using libimport to consume all testcases into a common aggregation class.

The advantage of this method of execution and ordering is controlled via the 4 digit number in the test function name, and can be striped across multiple files (a big need for my use case). Data can be passed between testcases (and suites) via setting pytest attributes (ie: pytest.mydata = 'foobar')

The disadvantage to this method, is that some functions / examples documented become unavailable directly to the testcase, which requires some jedi handwavy workarounds, but so far I have not had any direct limitiations.

And then there is the "Thar Be Dragons", which is every function in a suite file must be unique because they are all combined into a single inherited class. We have stubbed our toe due to duplicate topscope variables, duplicate testcase names etc... But so far I am quite happy with the implementation.

Each testcase file named tests/suites/{something}.py:

import pytest
class CommonTestClass():
    def test_0000_setup_something(self):
        pass
    def test_0010_test_something(self):
        pass

Super test class named tests/test_everything.py:

import os
from lib.utility import lookup_topology_tests

test_classname = 'CommonTestClass'
class TestSuper(*lookup_topology_tests(test_classname)):
    @classmethod
    def setupClass(self):
        pass

    @classmethod
    def tearDownClass(self):
        pass

lib/utility.py:

import glob
import importlib
import os
import unittest


def lookup_topology_tests(class_name):
    inherit_classes = [unittest.TestCase]
    modules = glob.glob(os.path.join(os.path.dirname(__file__), '../tests/suites/*.py'))
    for f in modules:
        base_name = os.path.splitext(os.path.basename(f))[0]
        module = importlib.import_module('tests.suites.' + base_name)
        try:
            inherit_classes.append(getattr(module, class_name))
        except AttributeError:
            pass

    return inherit_classes
==================================================================================================== test session starts ====================================================================================================
platform darwin -- Python 3.7.4, pytest-6.1.0, py-1.9.0, pluggy-0.13.1 -- 
...
collected 30 items                                                                                                                                                                                                          

tests/test_everything.py::TestSuper::test_0000_setup_test <- tests/suites/0000_setup.py PASSED                                                                                                          [  3%]
...
tests/test_everything.py::TestSuper::test_0010_mytest <- tests/suites/foo.py PASSED                                                                                                   [ 23%]
tests/test_everything.py::TestSuper::test_0010_myother <- tests/suites/bar.py PASSED                                                                                                             [ 26%]
...

james-powis avatar Nov 11 '20 19:11 james-powis

For what It is worth, I managed to obtain ordering by taking advantage of the alphabetical execution order for all test functions in an individual class via method naming convention and using libimport to consume all testcases into a common aggregation class.

This is pretty much my approach as well (plus pytest_collection_modifyitems). But sometimes it gets really ugly.

lukjak avatar Nov 11 '20 20:11 lukjak

I wonder, what is blocking this PR?

axel-h avatar Jan 24 '21 19:01 axel-h

There is a broken Travis check that exists on this PR, mostly because I pushed the last changes too fast, before the previous Travis build managed to complete. I could fix that by making an empty commit and pushing again, but the entire PR could as well be merged like this, considering this other check exists and passed without problems.

As much as I hate to say this, it appears that @RKrahl either haven't seen this PR yet, have seen it but didn't find the time to look at it, or just simply doesn't care. Either way, they could have at least left a response explaining their stance on it, instead of letting it hang with no response like this. Guess everyone will just have to keep waiting until something happens / changes.

DevilXD avatar Jan 25 '21 11:01 DevilXD

This one should be ready now too. I'd like to note that since there was no response for a long time, I've moved over to pytest-order for my project's needs already, but I understand that having a similar feature here would be cool too. And again, it could always be gated behind an option if needed.

DevilXD avatar Jan 08 '22 22:01 DevilXD

Actually, that would be my question: if pytest-order already does the job and even explicitly supports pytest-dependency, why should we duplicate that effort here?

I rather tend to solve this issue by improving the documentation, explaining how to use pytest-order together with pytest-dependency.

RKrahl avatar Jan 09 '22 08:01 RKrahl

For the record, pytest-order wasn't a thing when I was making this PR, as their project is only 10 months old. Since I've already implemented the sorting itself, updating this to the newest develop branch didn't really pose much issue to me - ultimately, it's up to you if you want to merge this or not. Personally, I think this functionality would be just fine being disabled by default, but gated behind an option. I can try adding this to the PR if you'd want me to.

DevilXD avatar Jan 09 '22 09:01 DevilXD

I'd like to add that my personal use case is to test an API wrapper, which has functionality working of which is dependent on other, more basic functionality. So, if this basic functionality fails to test, testing anything dependent on it doesn't really make any sense, as they'll fail with the exact same error. Setting up this plugin seemed great at first, but then I quickly realized that it's not smart enough to test the basic functionality first (by reordering the tests), making literally the entire purpose of it absolutely obsolete. Renaming or moving around the tests (as others suggested) is not a good solution to the problem, which is why this PR exists - reordering tests is the exact thing this plugin was missing this whole time, and installing another plugin just to "cover up" missing functionality doesn't sound as a good solution either.

If this is merged, or reordering based on dependencies is supported in any other way, I'm going back to this plugin in my project. pytest-order is cool, but I actually want to skip the dependent tests, and otherwise don't really care about actual test order, as long as this plugin can do it's job properly. Using both plugins is an option, but as I said, requiring another plugin just so this one works properly isn't a good solution.

DevilXD avatar Jan 09 '22 10:01 DevilXD

My 2 euro cents as the maintainer of pytest-order: As I wrote elsewhere, I actually like the philosophy of doing only one thing, but doing it well, which is exactly what is done with all the UNIX command line tools that work together. Needing another plugin should not be a big deal if they work together well. That being said, if this gets merged, I would probably still retain the functionality in pytest-order, because I don't want to destroy the ordering done by pytest-dependency. As of now, I don't move tests with dependency markers before tests they depend on by default, and with the --order-dependencies option also order such tests if needed. So the only thing I would have to do is to remove or deprecate this option and use it by default instead.

Note that I actually was about to write that I would remove the respective code from pytest-order, but while writing it realized that this would make the plugins incompatible...

As a side note to @RKrahl: have you considered transferring this plugin to the pytest-dev organization (as I did with pytest-order)? I think it would be a good match.

mrbean-bremen avatar Jan 09 '22 11:01 mrbean-bremen

Needing another plugin should not be a big deal if they work together well.

Yes, I agree. However, having test dependencies without actually making sure that those dependencies are evaluated first (by reordering the tests), kinda defeats the entire purpose of this plugin. I'm not trying to say having to use two plugins together is a bad thing, I'm just saying that in like 90% cases, the user will need to do this one way or another, unless they "solve" it with renaming or moving tests around. I'm actually surprised this isn't a feature of this plugin already, and wonder how on earth the author has managed to structure tests in their project, so that no reordering was required (unless they went with the other two approaches I mentioned). It's illogical to me that a plugin is literally useless without either special trickery or installing another plugin. Thank god that pytest-order exists now, because previously even that wasn't an option...

Note that I actually was about to write that I would remove the respective code from pytest-order, but while writing it realized that this would make the plugins incompatible...

If this would be gated behind an option, sorting could then be done from either of the two plugins. Or, if sorting would be the default here, then pytest-order would need to honor this ordering (otherwise tests would get skipped because they got misordered).

Again, this is really up to @RKrahl here. I think this plugin needs ordering to actually be useful to most users. If you'd be unsure, you can leave this PR open until someone finds it and gives some more feedback from their perspective, I'm fine with that. I've waited a year and a half for something to happen here, so I don't really mind waiting some more.

DevilXD avatar Jan 09 '22 12:01 DevilXD

I still prefer to keep the scope of the plugin focused. pytest-dependency is about skipping tests when running them makes no sense because the prerequisites are not in place. Reordering tests is really a different task. @DevilXD, you are right that pytest-order did not exist when you started this PR. But now, I am very happy that pytest-order is available so we can keep both tasks separate. I do believe that this is the better solution.

@DevilXD you write that pytest-dependency would be obsolete without the ordering feature and that you don't understand how I would have managed to structure tests in my projects, so that no reordering was required. This is very simple, as pytest by default runs tests in a well defined order: it runs the test modules one by one in alphabetic order and the tests within a module in the order they appear in the source. (Unless you explicitely group tests using fixtures.) I never found it particularly challenging to write tests in the proper order. In fact, if I just write them down in a somewhat logic structure, they end up just naturally in the proper execution order. I never felt like needing a tool to reorder them. Still, I appreciate that there may be cases that such a tool would be useful and so I'm glad that it already exists with pytest-order.

So the bottom line is, if you need reordering based on dependencies, I'd recommend to use pytest-dependency and pytest-order in combination. The remaining question is, whether there is anything missing in that combo that dosn't work well. If yes, I'd be happy to talk to the authors of pytest-order to work together to fix it.

RKrahl avatar Jan 09 '22 13:01 RKrahl

The remaining question is, whether there is anything missing in that combo that dosn't work well. If yes, I'd be happy to talk to the authors of pytest-order to work together to fix it.

I'm sure there are some cases that are not covered in pytest-order, but if such a case will be found, it shall be filed as a bug in pytest-order - I will happily try to fix it, and seek your help if needed.

mrbean-bremen avatar Jan 09 '22 13:01 mrbean-bremen

I'm sure there are some cases that are not covered in pytest-order, but if such a case will be found, it shall be filed as a bug in pytest-order - I will happily try to fix it, and seek your help if needed.

One thing is for sure and that is AFAIR documented in pytest-order: if a dependency is not set in the marker but by using the depends() function. I believe there is now way to detect and properly handle this.

RKrahl avatar Jan 09 '22 13:01 RKrahl

One thing is for sure and that is AFAIR documented in pytest-order: if a dependency is not set in the marker but by using the depends() function. I believe there is now way to detect and properly handle this.

Yes, you are right. I am aware of this (or was - had already forgotten this), and this is something that I'm not sure how to handle. This makes pytest-dependency quite flexible, but I'm not sure if it makes even sense to try to add this to the ordering code. Until there is no explicit demand for this, I would probably not even look into this.

mrbean-bremen avatar Jan 09 '22 13:01 mrbean-bremen

I will probably go over the pytest-dependency documentation again and check for any features that are not handled in ordering, and adapt the documentation accordingly.

mrbean-bremen avatar Jan 09 '22 13:01 mrbean-bremen

Reordering tests is really a different task.

It may be a different task, but it is a part of the same feature. It (reordering) would be not necessary, if pytest-dependecy had supported skipping only in case of failure of immediately preceding step in pytest order (physical order of steps in a test file).

lukjak avatar Jan 09 '22 13:01 lukjak

This is very simple, as pytest by default runs tests in a well defined order: it runs the test modules one by one in alphabetic order and the tests within a module in the order they appear in the source. (Unless you explicitely group tests using fixtures.) I never found it particularly challenging to write tests in the proper order. In fact, if I just write them down in a somewhat logic structure, they end up just naturally in the proper execution order. I never felt like needing a tool to reorder them.

So essentially, you're saying that you've made and published this plugin with the sole purpose of helping your use case only, and anyone else who've structured their tests even a little bit out of the "natural order" you're claiming here (most likely the majority of this plugin users) is SOL and has to rely on other plugins to complete the same job? Again, this plugin is literally useless if the dependent tests aren't ran in the correct order, and in my case, the tests are logically grouped together in files, and I don't really care which order they execute in, as long as this plugin can do it's very damned purpose it's been designed for - but it doesn't.

Note that I don't need any particular order these need to run in on itself, as I said, I don't care about ordering, as long as this plugin works. I don't need ordering, I need dependency. Dependency implies the dependencies are going to be ran first, otherwise you have no way of knowing if the dependent test is allowed to run or not. This plugin is literally half-done, and it seems like you're too blind to see it.

sigh

Can you at least put a disclaimer in README that reordering tests is not and won't be supported, and direct people to pytest-order instead?

DevilXD avatar Jan 09 '22 16:01 DevilXD

@DevilXD maybe https://pypi.org/project/pytest-depends (from https://gitlab.com/MaienM/pytest-depends) is what you are looking for?

axel-h avatar Jan 09 '22 16:01 axel-h

Again, this plugin is literally useless

Please take the tone down a bit, ranting is not helpful and will lead nowhere. There are obviously many users that do not have this problem. You can always make a fork of this package, if it does not suit you.

pytest-depends is indeed another option. It has less features compared to pytest-dependency, but it does order the tests, so you can use it instead. I myself won't use it, because it changes the test order if installed, and not in a deterministic way, so it does not play nice with tests that rely on the given order, or with other sorting plugins.

mrbean-bremen avatar Jan 09 '22 17:01 mrbean-bremen

@axel-h The topological sort it uses requires installing a whole networking library of some sorts (like, why?), and the sorting itself isn't stable, as the other person noted. My absolutely best option is to either use both this and pytest-order plugins together, or stick to what I have with pytest-order and the -x option (other tests won't run if the testing ends on first failed "dependency").

Also to @mrbean-bremen: I'm saying the truth here - this plugin is useless if the dependencies aren't executed before dependent tests, and apparently it's author expects the programmer to work around it ourselves (by moving tests around to follow "natural order", or using another plugin). It's fine, but I think there needs to be fair warning/note placed in the README explaining this. I've had higher expectations when I first discovered this plugin, got disappointed when I learned it's not doing it's job properly, wanted to help by creating this PR, and waited a year and a half only to be disappointed even further. I simply don't want others to try it, wonder why it's not doing what they expected it to, only to learn it doesn't really work on it's own. It'd only be frustrating and a waste of time.

DevilXD avatar Jan 09 '22 18:01 DevilXD