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

Support for reordering tests based on dependencies #44

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from

Conversation

DevilXD
Copy link

@DevilXD DevilXD commented Jun 10, 2020

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
Copy link
Author

DevilXD commented Jun 10, 2020

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

@6lor
Copy link

6lor commented Jun 11, 2020

@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.

@DevilXD
Copy link
Author

DevilXD commented Jun 11, 2020

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
Copy link
Author

DevilXD commented Jun 15, 2020

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 DevilXD marked this pull request as draft June 15, 2020 08:09
@DevilXD DevilXD marked this pull request as ready for review June 15, 2020 20:42
@DevilXD
Copy link
Author

DevilXD commented Jun 15, 2020

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
Copy link
Author

DevilXD commented Jun 17, 2020

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 DevilXD marked this pull request as draft June 17, 2020 07:31
@DevilXD DevilXD marked this pull request as ready for review June 17, 2020 17:04
@DevilXD
Copy link
Author

DevilXD commented Jan 8, 2022

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.

@RKrahl
Copy link
Owner

RKrahl commented Jan 9, 2022

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.

@DevilXD
Copy link
Author

DevilXD commented Jan 9, 2022

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
Copy link
Author

DevilXD commented Jan 9, 2022

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.

@mrbean-bremen
Copy link

mrbean-bremen commented Jan 9, 2022

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.

@DevilXD
Copy link
Author

DevilXD commented Jan 9, 2022

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.

@RKrahl
Copy link
Owner

RKrahl commented Jan 9, 2022

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.

@mrbean-bremen
Copy link

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.

@RKrahl
Copy link
Owner

RKrahl commented Jan 9, 2022

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.

@mrbean-bremen
Copy link

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
Copy link

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.

@lukjak
Copy link

lukjak commented Jan 9, 2022

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).

@DevilXD
Copy link
Author

DevilXD commented Jan 9, 2022

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?

@axel-h
Copy link

axel-h commented Jan 9, 2022

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

@mrbean-bremen
Copy link

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.

@DevilXD
Copy link
Author

DevilXD commented Jan 9, 2022

@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.

@mrbean-bremen
Copy link

I'm saying the truth here

You are stating your opinion. It may or may not be the same opinion as other people, especially the owner of the repo, have. It usually does not help a discussion to see an opinion as "the truth", even if you are frustrated - though that is something that seems to be the default nowadays (ranting about the discussion culture also does not help, ah well...). But I will shut up now. It is up to the owner to decide what he does with his code.

@lukjak
Copy link

lukjak commented Jan 9, 2022

It is up to the owner to decide what he does with his code

This is probably the only thing right about your stance.
If RKrahl does not want to include ordering because it's a "different thing" from "conditional skipping" (yes, I know it is) - it's his full right. But not including ready, useful feature does not seem OK. Especially after 16 months of PR being here.
For some notion of "functional purity"? As DevilXD says, pytest-dependency is not complete without ordering, even if it works for a lot of people in some cases. Would you consider calculator which adds correctly unless you use negative numbers as inputs feature-complete? It would work perfectly for all millions of people working in trade - you never have negative amount of something.
Same here - setting dependencies actually defines execution order - if you say A depends on B, you actually define ordering - potentially other than defined natively (physical ordering in the file). And in some more complicated/dynamic scenarios lack of support for doing the correct ordering breaks the whole idea and makes pytest-dependency not possible to use (without lots of extra hooplas).
pytest-depends is basically broken from my perspective, and pytest-order makes me break DRY rule by forcing to define ordering twice, which is bad by itself for all the known reasons.

@mgheorghe
Copy link

mgheorghe commented Apr 27, 2023

i would like also this merged in .
i would like if dependency module will make sure the depended on case is executed before the case that depends on it.
i don't want to hardcode the order, i want this to be automatically derived from the dependency graph.

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.

Reorder test execution based on dependencies?
8 participants