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

Add a scope to dependencies #35

Merged
merged 19 commits into from
Feb 10, 2020
Merged

Add a scope to dependencies #35

merged 19 commits into from
Feb 10, 2020

Conversation

RKrahl
Copy link
Owner

@RKrahl RKrahl commented Jan 1, 2020

This adds a scope argument to the pytest.mark.dependency marker. The values may be either "session", "module", or "class". The references to other tests in the depends argument will be taken with respect to the scope indicated in the scope argument. The scope defaults to "module" which is the current behavior of pytest-dependency.

This implements and closes #3. It supersedes and closes #29.

JoeSc and others added 3 commits January 1, 2020 15:47
By using different scope="" arguments in the dependency mark the user
can depend on tests in a different module.  Or by using scope="session,
depends="all" a test can be skipped if ANY previous test has failed.
When depending on a test outside the module its full name must be used
i.e. tests/test_a.py::test_a
@RKrahl RKrahl added the enhancement New feature or request label Jan 1, 2020
@RKrahl RKrahl added this to the 0.5 milestone Jan 1, 2020
@RKrahl
Copy link
Owner Author

RKrahl commented Jan 1, 2020

Still to do:

  • Verify consistency. There are still some corner cases to check.
  • Code review and some minor cleanup.
  • Add the scope argument also to the depends() function.
  • Review tests, add tests for all relevant cases.
  • Documentation.

@RKrahl
Copy link
Owner Author

RKrahl commented Jan 4, 2020

There is an issue with using the pytest nodeid as the default name of tests in session scope: it depends on the invocation of pytest. In the simplest case, the nodeid consists of two parts, separated by ::, the path of the test module and the name of the test function. The module path is taken relative to the rootdir. As a result, it may happen that all references to dependencies in session scope will break if the effective rootdir during pytest invocation differs from what the author of the tests had envisaged.

As an example, consider a subdirectory tests with a test module test_foo.py having a test function test_a(). If you call pytest with no arguments, the nodeid will be tests/test_foo.py::test_a. Now if you change into that tests directory and call pytest there, it will be test_foo.py::test_a.

I shortly considered to replace the module path in the nodeid with the module name, e.g. stripping the directories. But this would create another problem that the module name may not be unique if there are two different modules with the same name in different subdirectories. The pros and cons of both alternatives:

  • the module path may depend on pytest invocation.
  • the module name may not be unique.
  • the unchanged nodeid with the module path is simpler in the implementation.
  • the unchanged nodeid with the module path is consistent with pytest documentation and is displayed when pytest is called with -v. Deviating from that will make pytest-dependency documentation significantly more complicated.

In the end, I opted for simplicity and keep the nodeids unchanged. But the issue with the dependency from the invocation will require appropriate documentation.

@RKrahl
Copy link
Owner Author

RKrahl commented Jan 5, 2020

The hierarchy of nodes in pytest is session (-> package) -> module (-> class -> instance) -> function, where package, class, and instance are only present if applicable. Yet this PR (up to now) only adds session, module, and class as scope. I wonder if we should also add package and instance. (Scope function doesn't make sense when we speak of dependencies between functions.)

I guess package is rarely used in practice, but could certainly be added just for the sake of completeness.

The instance scope is trickier: an instance node seem to be always present for class methods and seem to represent the instance of that class (the pytest documentation on these internal data structures is sketchy at best). In all situations that I am aware of, a test class has only one single instance. In that case, instance scope would be redundant with class scope. If there would be more then one instance of a class, class scope for pytest-dependency would most likely not work properly and instance scope should be used instead. I'm not sure if a test class can have more then one instance, though. I could see three possible options for dealing with class versus instance scope:

  1. Keep it as is now, e.g. disregard the instance level. Drawback: would most likely not work properly if a class has more then one instance.
  2. Add instance scope next to class scope. Drawback: confusing and redundant in most (all?) practical cases.
  3. Keep class scope, but silently turn it into an instance scope, e.g. attach it to the instance node, rather then to the class node. Drawback: inconsistent and a nightmare for the documentation.

For the moment, I'm inclined to go for option 1 and wait and see if any problems arise in practice. That would also be in line with pytest core (possible values for the scope argument in pytest fixtures are: function, class, module, package or session).

@RKrahl RKrahl changed the base branch from master to develop January 26, 2020 22:40
@RKrahl RKrahl marked this pull request as ready for review February 10, 2020 15:38
DependencyManager.getManager().
@RKrahl RKrahl merged commit 0b40d65 into develop Feb 10, 2020
@RKrahl RKrahl deleted the scope branch February 10, 2020 15:51
@CMCDragonkai
Copy link

Hooray!

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 this pull request may close these issues.

Add a way to configure the scope
3 participants