#3034 Added new option "--new-first" #3218

feuillemorte · 2018-02-13T20:02:01Z

Fixes #3034
Added new option --new-first

How it works:
Create different test files:
tests
|- test_one.py
- test_two.py

Every file contains two tests:

def test_1():
    assert 1

def test_2():
    assert 1

run command

pytest tests -v

result:

tests/test_one.py::test_1 PASSED
tests/test_one.py::test_2 PASSED
tests/test_two.py::test_1 PASSED
tests/test_two.py::test_2 PASSED

run command

pytest tests -v --nf

result:

tests/test_two.py::test_1 PASSED
tests/test_two.py::test_2 PASSED
tests/test_one.py::test_1 PASSED
tests/test_one.py::test_2 PASSED

Order changed to modifyed files first

Add new test to test_two.py file and then modify test_one.py file

run command

pytest tests -v --nf

result:

tests/test_two.py::test_3 PASSED
tests/test_one.py::test_1 PASSED
tests/test_one.py::test_2 PASSED
tests/test_two.py::test_1 PASSED
tests/test_two.py::test_2 PASSED

Order changed: first new test, then modified files

coveralls · 2018-02-13T20:32:27Z

Coverage increased (+0.07%) to 92.593% when pulling c032d4c on feuillemorte:3034-new-tests-first into 0a3c80e on pytest-dev:features.

nicoddemus

Awesome work @feuillemorte, this is a great first pass. Please take a look at my comments and see what you think. 👍

nicoddemus · 2018-02-15T22:04:38Z

_pytest/cacheprovider.py

+    def __init__(self, config):
+        self.config = config
+        self.active = config.option.newfirst
+        self.all_items = config.cache.get("cache/allitems", {})


I believe the default value here should be [], for consistency more than anything else.

nicoddemus · 2018-02-15T22:06:59Z

_pytest/cacheprovider.py

+            other_items = []
+            for item in items:
+                mod_timestamp = os.path.getmtime(str(item.fspath))
+                if self.all_items and item.nodeid not in self.all_items:


Unfortunately item.nodeid not in self.all_items will be a linear search, which can be expensive if we have a large collection.

I think you should change new_items and other_items to a dict(nodeid -> timestamp) instead, given that you will be sorting them later anyway.

Also you can check just with if item.nodeid not in self.all_items: because and empty list or dict will return False for the containment check.

the code is incorrect about the typo of all_items, it changes multiple times

@RonnyPfannschmidt You mean I need to rename it to smth like "cached_items"?

@nicoddemus

I think you should change new_items and other_items to a dict(nodeid -> timestamp) instead, given that you will be sorting them later anyway.

In this case I will not know about new tests, isn't it? Only about modified files

In this case I will not know about new tests, isn't it? Only about modified files

I did not explain correctly sorry, I meant this:

new_items = {} other_items = {} for item in items: if item.nodeid not in self.all_items: new_items[item.nodeid] = item else: other_items[item.nodeid] = item

And you can get the timestamp during your sorting function:

items[:] = self._get_increasing_order(six.itervalues(new_items)) + \ self._get_increasing_order(six.itervalues(other_items.values)) ... def _get_increasing_order(self, items): return sorted(items, key=lambda item: item.fspath.mtime(), reverse=True)

@nicoddemus this is not working in py27. Every time different order:

test_2/test_2.py::test_1[1] PASSED test_2/test_2.py::test_1[2] PASSED test_1/test_1.py::test_1[2] PASSED test_1/test_1.py::test_1[1] PASSED

I think it's better to save my code based on lists. What do you think?

Try changing new_items and other_items to a OrderedDict instead, that should be equivalent to the previous code.

nicoddemus · 2018-02-15T22:08:54Z

_pytest/cacheprovider.py

+        if config.getoption("cacheshow") or hasattr(config, "slaveinput"):
+            return
+        config.cache.set("cache/allitems",
+                         [item.nodeid for item in self.all_items if isinstance(item, Function)])


Why are you testing isinstance here? 🤔

Please, check testing/acceptance_test.py:584

pytest/testing/acceptance_test.py

Lines 580 to 585 in 063e2da

monkeypatch.setenv('PYTHONPATH', join_pythonpath(testdir))

result = testdir.runpytest("--pyargs", "tpkg.test_missing", syspathinsert=True)

assert result.ret != 0

result.stderr.fnmatch_lines([

"*not*found*test_missing*",

])

It failes because sometimes item is a string, not object:

AttributeError: 'unicode' object has no attribute 'nodeid'

I see thanks. Sometimes pytest_collect_modifyitems can received strings, weird. Didn't take the time to investigate this further, but regardless I think it is safer to check for pytest.Item instead while still in pytest_collect_modifyitems:

self.cached_nodeids = [x.nodeid for x in items if isinstance(x, pytest.Item)]

(Also already suggesting renaming all_items to cached_nodeids). During pytest_sessionfinish you can dump it directly:

config.cache.set("cache/allitems", self.cached_nodeids )

nicoddemus · 2018-02-15T22:10:43Z

_pytest/cacheprovider.py

@@ -179,6 +216,10 @@ def pytest_addoption(parser):
        help="run all tests but run the last failures first.  "
             "This may re-order tests and thus lead to "
             "repeated fixture setup/teardown")
+    group.addoption(
+        '--nf', '--new-first', action='store_true', dest="newfirst",
+        help="run all tests but run new tests first, then tests from "


I think this reads better: run tests from new files first, then the rest of the tests sorted by file mtime

nicoddemus · 2018-02-15T22:12:10Z

doc/en/cache.rst

+New tests first
+-----------------
+
+The plugin provides command line options to run tests in another order:


I think this should be moved closer to --lf, right before the The new config.cache object section.

I also think we should use the same blurb from the changelog. 👍

nicoddemus · 2018-02-15T22:16:38Z

changelog/3034.feature

@@ -0,0 +1 @@
+Added new option `--nf`, `--new-first`. This option enables run tests in next order: first new tests, then last modified files with tests in descending order (default order inside file).


Suggestion:

New ``--nf``, ``--new-first`` options: run new tests first followed by the rest of the tests, in both cases tests are also sorted by the file modified time, with more recent files coming first.

nicoddemus · 2018-02-15T22:23:18Z

testing/test_cacheprovider.py

+
+class TestNewFirst(object):
+    def test_newfirst_usecase(self, testdir):
+        t1 = testdir.mkdir("test_1")


You can write those files more easily with makepyfile:

testdir.makepyfile(**{ 'test_1/test_1.py': ''' def test_1(): assert 1 def test_2(): assert 1 def test_3(): assert 1 ''', 'test_1/test_2.py': ''' def test_1(): assert 1 def test_2(): assert 1 def test_3(): assert 1 ''' })

nicoddemus · 2018-02-15T22:26:39Z

testing/test_cacheprovider.py

+        )
+
+        path_to_test_1 = str('{}/test_1/test_1.py'.format(testdir.tmpdir))
+        os.utime(path_to_test_1, (1, 1))


You can use setmtime here for the same effect.

testdir.tmpdir.join('test_1/test_1.py').setmtime(1)

nicoddemus · 2018-02-15T22:33:06Z

testing/test_cacheprovider.py

+        ])
+
+    def test_newfirst_parametrize(self, testdir):
+        t1 = testdir.mkdir("test_1")


Same suggestions here about using testdir. 👍

feuillemorte · 2018-02-16T07:08:33Z

_pytest/cacheprovider.py

+            new_items = []
+            other_items = []
+            for item in items:
+                mod_timestamp = os.path.getmtime(str(item.fspath))


@nicoddemus
Will it work on windows? Could you test if you work on windows, please?

RonnyPfannschmidt · 2018-02-16T09:02:31Z

_pytest/cacheprovider.py

+            new_items = []
+            other_items = []
+            for item in items:
+                mod_timestamp = os.path.getmtime(str(item.fspath))


use item.fspath.mtime() here - it works on windows btw, the tests passedthere at least

RonnyPfannschmidt · 2018-02-16T09:04:51Z

_pytest/cacheprovider.py

+            other_items = []
+            for item in items:
+                mod_timestamp = os.path.getmtime(str(item.fspath))
+                if self.all_items and item.nodeid not in self.all_items:


the code is incorrect about the typo of all_items, it changes multiple times

nicoddemus · 2018-02-19T21:32:05Z

_pytest/cacheprovider.py

+            other_items = []
+            for item in items:
+                mod_timestamp = os.path.getmtime(str(item.fspath))
+                if self.all_items and item.nodeid not in self.all_items:


In this case I will not know about new tests, isn't it? Only about modified files

I did not explain correctly sorry, I meant this:

new_items = {} other_items = {} for item in items: if item.nodeid not in self.all_items: new_items[item.nodeid] = item else: other_items[item.nodeid] = item

And you can get the timestamp during your sorting function:

items[:] = self._get_increasing_order(six.itervalues(new_items)) + \ self._get_increasing_order(six.itervalues(other_items.values)) ... def _get_increasing_order(self, items): return sorted(items, key=lambda item: item.fspath.mtime(), reverse=True)

nicoddemus · 2018-02-19T21:49:42Z

_pytest/cacheprovider.py

+        if config.getoption("cacheshow") or hasattr(config, "slaveinput"):
+            return
+        config.cache.set("cache/allitems",
+                         [item.nodeid for item in self.all_items if isinstance(item, Function)])


I see thanks. Sometimes pytest_collect_modifyitems can received strings, weird. Didn't take the time to investigate this further, but regardless I think it is safer to check for pytest.Item instead while still in pytest_collect_modifyitems:

self.cached_nodeids = [x.nodeid for x in items if isinstance(x, pytest.Item)]

(Also already suggesting renaming all_items to cached_nodeids). During pytest_sessionfinish you can dump it directly:

config.cache.set("cache/allitems", self.cached_nodeids )

…o 3034-new-tests-first

nicoddemus

Thanks @feuillemorte!

feuillemorte · 2018-03-02T10:42:42Z

@RonnyPfannschmidt please, review :)

blueyed · 2018-04-08T21:12:45Z

testing/test_cacheprovider.py

@@ -56,7 +56,7 @@ def test_error():
        assert result.ret == 1
        result.stdout.fnmatch_lines([
            "*could not create cache path*",
-            "*1 warnings*",
+            "*2 warnings*",


What is the new warning here? Shouldn't it get matched also?

1st warning is

could not create cache path .pytest_cache/v/cache/lastfailed

and second is for new-first. don't need to create another test for it

#3034 Added new option "--new-first"

dff0500

nicoddemus requested changes Feb 15, 2018

View reviewed changes

feuillemorte commented Feb 16, 2018

View reviewed changes

RonnyPfannschmidt requested changes Feb 16, 2018

View reviewed changes

nicoddemus requested changes Feb 19, 2018

View reviewed changes

feuillemorte added 2 commits February 23, 2018 22:49

#3034 Fix comments

e865f2a

Merge branch 'features' of https://github.com/feuillemorte/pytest int…

c032d4c

…o 3034-new-tests-first

nicoddemus approved these changes Feb 26, 2018

View reviewed changes

RonnyPfannschmidt approved these changes Mar 2, 2018

View reviewed changes

nicoddemus merged commit 0883139 into pytest-dev:features Mar 2, 2018

blueyed reviewed Apr 8, 2018

View reviewed changes

feuillemorte deleted the 3034-new-tests-first branch April 8, 2018 21:23

blueyed mentioned this pull request Oct 19, 2019

cache: NFPlugin: keep known nodeids #6004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3034 Added new option "--new-first" #3218

#3034 Added new option "--new-first" #3218

feuillemorte commented Feb 13, 2018

coveralls commented Feb 13, 2018 •

edited

Loading

nicoddemus left a comment

nicoddemus Feb 15, 2018

nicoddemus Feb 15, 2018

RonnyPfannschmidt Feb 16, 2018

feuillemorte Feb 16, 2018

feuillemorte Feb 16, 2018

nicoddemus Feb 19, 2018

feuillemorte Feb 22, 2018

nicoddemus Feb 22, 2018

nicoddemus Feb 15, 2018

feuillemorte Feb 16, 2018

nicoddemus Feb 19, 2018

nicoddemus Feb 15, 2018

nicoddemus Feb 15, 2018

nicoddemus Feb 15, 2018

nicoddemus Feb 15, 2018

nicoddemus Feb 15, 2018

nicoddemus Feb 15, 2018

nicoddemus Feb 15, 2018

feuillemorte Feb 16, 2018

RonnyPfannschmidt Feb 16, 2018

RonnyPfannschmidt Feb 16, 2018

nicoddemus Feb 19, 2018

nicoddemus Feb 19, 2018

nicoddemus left a comment

feuillemorte commented Mar 2, 2018

blueyed Apr 8, 2018

feuillemorte Apr 8, 2018 •

edited

Loading

	monkeypatch.setenv('PYTHONPATH', join_pythonpath(testdir))
	result = testdir.runpytest("--pyargs", "tpkg.test_missing", syspathinsert=True)
	assert result.ret != 0
	result.stderr.fnmatch_lines([
	"notfoundtest_missing",
	])

		@@ -0,0 +1 @@
		Added new option `--nf`, `--new-first`. This option enables run tests in next order: first new tests, then last modified files with tests in descending order (default order inside file).

#3034 Added new option "--new-first" #3218

#3034 Added new option "--new-first" #3218

Conversation

feuillemorte commented Feb 13, 2018

coveralls commented Feb 13, 2018 • edited Loading

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

feuillemorte commented Mar 2, 2018

Choose a reason for hiding this comment

feuillemorte Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

coveralls commented Feb 13, 2018 •

edited

Loading

feuillemorte Apr 8, 2018 •

edited

Loading