Nose cleanup first step #13206

sgaist · 2021-10-20T21:50:58Z

This merge request goes through the test files and makes a first pass cleanup to remove the direct use of nose in the test files themselves.

Each file has been cleaned up independently so that it can be reviewed in an easy fashion.

Part of #13108

Carreau

A couple of comments, I think you forgot one assert.

I'm also trying to see if it's possible to rebase and and applying the reformat hook to each commit, so that it does not complain in the test suite. If that works i'll point you to the rebased branch and the reformat commit, and we can decide if we ant to squash them or not.

Feel free to also do many smaller PRs, that might allow me to quickly review them and merge when I'm in between two compilations or waiting on another test suite.

, to see if I can get the test to pass, and

Carreau · 2021-10-21T00:34:07Z

IPython/core/tests/test_completer.py

@@ -329,16 +326,16 @@ def test_no_ascii_back_completion(self):
            # single ascii letter that don't have yet completions
            for letter in "jJ":
                name, matches = ip.complete("\\" + letter)
-                nt.assert_equal(matches, [])
+                self.assertEqual(matches, [])


if it's always skipped we might as well delete the test ?
Can be done in separately, just taking notes.

Agreed

I think that a separate step is better to keep things clear.

IPython/core/tests/test_display.py

Carreau · 2021-10-21T00:38:21Z

IPython/core/tests/test_display.py

-    nt.assert_raises(ValueError, display.Image)
-    nt.assert_raises(ValueError, display.Image, data='this is not an image', format='badformat', embed=True)
+    pytest.raises(ValueError, display.Image)
+    pytest.raises(ValueError, display.Image, data='this is not an image', format='badformat', embed=True)


👍 I wasn't aware you could use pytest raises like so.

IPython/core/tests/test_display.py

Carreau · 2021-10-21T00:46:32Z

IPython/core/tests/test_inputsplitter.py


    def test_cellmagic_help(self):
        self.sp.push('%%cellm?')
-        nt.assert_false(self.sp.push_accepts_more())
+        assert not self.sp.push_accepts_more()


Philosophical question, is nt.assert_false equivalent to that, or equivalent to is false, because:

>>> not '' True

? I think that's fine here, but just leaving my own questioning here.

I guess it's the same for assert vs assert_true...

You raise a good point.

Being more explicit with is false and is true will also makes the expected output of the method clearer. I'll update the checks.

Carreau · 2021-10-21T01:00:02Z

IPython/core/tests/test_pylabtools.py

 from matplotlib import pyplot as plt
 import matplotlib_inline
+from matplotlib_inline import backend_inline


did you meant to do some more refactor ?

Not exactly, on my machine, if backend_inline is not explicitly imported, I get AttributeError: module 'matplotlib_inline' has no attribute 'backend_inline'.

It might be because I installed matplotlib through conda rather than pip.

@martinRenou ? Want to add ad from . import backend_inline in __init__.py ?

IPython/core/tests/test_splitinput.py

Carreau · 2021-10-21T02:08:10Z

I want to make sure @Kojoley sees that and does not do too much of duplicate work.

My computer is still rebasing and applying the auto formatter, Will post updates when it's finished.

Carreau · 2021-10-21T02:20:31Z

You can check this branch https://github.com/Carreau/ipython/tree/nose_cleanup_first_step, it is the same as your except every-other commit run the reformat step.

If you want I can either push it as it, or squash every other commits to be part of what you did, and I can also push to your branch. Let me know if you want me to do that.

Kojoley · 2021-10-21T02:33:35Z

On this kind of changes it is useful to check code coverage for regressions. I see that CI is uploading coverage to codecov https://codecov.io/github/ipython/ipython/commit/fdee94ec9e877777cd30beccb1b42e744fb42d10 but no info is added to the PR status, is it intentionally?

Carreau · 2021-10-21T02:37:43Z

is it intentionally?

I don't quite remember, I believe that when we originally added codecov the bot was poting a new comment on each push/each time a new CI item would finish. I don't believe it's the case anymore so we can re-enable. I'll see if I can do
that.

Sidenote, there is a conflict now.

Kojoley · 2021-10-21T02:42:24Z

I don't quite remember, I believe that when we originally added codecov the bot was poting a new comment on each push/each time a new CI item would finish.

Yeah, that bot could be quite annoying, but it is not required for the status updates through API that just adds a line with code coverage stats in PR "checks" status box.

Carreau · 2021-10-21T02:53:35Z

I'm not quite sure why the status is not posted.

I'm going to try things in #13207

sgaist · 2021-10-21T20:58:58Z

With regard to darker, what about introducing pre-commit hooks to the repository ?

Carreau · 2021-10-22T00:12:29Z

With regard to darker, what about introducing pre-commit hooks to the repository ?

I think it would be ok to add. I have it personally as an editor hook so I don't need a pre-commit hook.

But I'm also a bit worried it is as disruptive for users, in another project I work on, the pre-commit hook takes 5 sec, and every other time prevent me from committing even if I know i don't care about having all the files formatted, and no unused variables.

So I'd be happy to have one, but I don't want to enforce it if it disturb user.

in this particular PR, as you seem to be fairly familiar with git and it touched a bunch of files, I pushed to have each commit properly formatted, but on a new contributor, and smaller PRS I would have just pushed a single formatting commit before merging.

sgaist · 2021-10-23T08:04:41Z

I am all for properly formatted code. As matter of fact I wanted to ask about doing a round of black/isort run once the move to pytest was done.

For pre-commit hooks, there's nothing to worry about for newcomers. If you don't run pre-commit install they won't be called at all.

However, I think that encouraging them to use it will also help them learn the tool which is beneficial.

Needs more work to remove all nose related code

sgaist · 2021-10-23T15:48:13Z

I think there might be some issue with code coverage as it looks like Windows is not taken into account and there are quite a lot of tests that are skipped of it.

Carreau · 2021-10-25T00:12:46Z

I think there might be some issue with code coverage as it looks like Windows is not taken into account and there are quite a lot of tests that are skipped of it.

Don't worry about github and coverage for now unless there is a delta which is gigantic.
There is a lot of code for weird corner case that don't get covered and might not be necessary.

Carreau · 2021-10-25T00:13:09Z

.pre-commit-config.yaml

+    rev: 1.3.1
+    hooks:
+    -   id: darker
+


+1 awesome.

Carreau · 2021-10-26T02:52:01Z

IPython/core/tests/test_pylabtools.py

 from matplotlib import pyplot as plt
 import matplotlib_inline
+from matplotlib_inline import backend_inline


@martinRenou ? Want to add ad from . import backend_inline in __init__.py ?

Carreau · 2021-10-26T02:56:28Z

Ok, let's get that in.

Carreau · 2021-10-26T02:56:41Z

Thanks !

Carreau requested changes Oct 21, 2021

View reviewed changes

sgaist and others added 20 commits October 23, 2021 13:44

[setup] Add pytest to test dependencies

ebeba47

[pre-commit] Add initial configuration

c814242

[testing][tests][decorators] Remove nose

cf20712

[testing][tests][tools] Remove nose

6b1a3e4

[core][tests][application] Remove nose

1efb051

[core][tests][async_helpers] Remove nose

4ac9d7f

[core][tests][completer] Remove nose

322441c

[core][tests][completerlib] Remove nose

44d74a9

[core][tests][debugger] Remove nose

7880f76

[core][tests][display] Remove nose

f4d8de4

[core][tests][events] Remove nose

8f53001

[core][tests][extension] Remove nose

1304d9e

[core][tests][formatters] Remove nose

2de4d8b

[core][tests][handlers] Remove nose

2965bcf

[core][tests][history] Remove nose

cf762a5

[core][tests][hooks] Remove nose

0ee3808

[core][tests][inputsplitter] Remove nose

8903f79

[core][tests][inputtransformer] Remove nose

3bf77e7

[core][tests][inputtransformer2] Remove nose

b75105d

[core][tests][interactiveshell] Remove nose

85ad41c

sgaist added 10 commits October 23, 2021 14:01

[lib][tests][deepreload] Remove nose

a1ddd3b

[lib][tests][latextools] Remove nose

e01e332

[lib][tests][pretty] Remove nose

3c04a29

[terminal][tests][embed] Remove nose

a167c8b

[terminal][tests][interactiveshell] Remove nose

66e3f6e

[utils][tests][dir2] Remove nose

b7155f6

[utils][tests][importstring] Remove nose

77e8177

[utils][tests][io] Remove nose

c8161eb

[utils][tests][module_paths] Remove nose

691cde9

[utils][tests][openpy] Remove nose

9c38a13

sgaist requested a review from Carreau October 23, 2021 12:07

sgaist added 7 commits October 23, 2021 16:51

[utils][tests][path] Remove the use of nt

c179da0

Needs more work to remove all nose related code

[utils][tests][process] Remove nose

88d2ced

[utils][tests][pycolorize] Remove nose

bd2c171

[utils][tests][sysinfo] Remove nose

8be458e

[utils][tests][text] Remove nose

45fe9d8

[utils][tests][tokenutil] Remove nose

01716cb

[utils][tests][dir2] Improve test_misbehaving_object_without_trait_names

7fb86c5

Carreau added this to the 8.0 milestone Oct 25, 2021

Carreau mentioned this pull request Oct 26, 2021

Remove always skipped test. #13219

Closed

Carreau approved these changes Oct 26, 2021

View reviewed changes

Carreau mentioned this pull request Oct 26, 2021

Add from . import backend_inline in __init__.py ? ipython/matplotlib-inline#8

Closed

Carreau merged commit 24290cb into ipython:master Oct 26, 2021

sgaist deleted the nose_cleanup_first_step branch October 26, 2021 13:53

Kojoley mentioned this pull request Nov 15, 2021

[WIP] Initial port of ipython tests from nose to pytest. #12840

Closed

Carreau added the 8.0 what's new label Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nose cleanup first step #13206

Nose cleanup first step #13206

sgaist commented Oct 20, 2021

Carreau left a comment

Carreau Oct 21, 2021

sgaist Oct 21, 2021

Carreau Oct 21, 2021

Carreau Oct 21, 2021

Carreau Oct 21, 2021

sgaist Oct 21, 2021

Carreau Oct 21, 2021

sgaist Oct 21, 2021

Carreau Oct 26, 2021

Carreau commented Oct 21, 2021

Carreau commented Oct 21, 2021

Kojoley commented Oct 21, 2021

Carreau commented Oct 21, 2021

Kojoley commented Oct 21, 2021

Carreau commented Oct 21, 2021

sgaist commented Oct 21, 2021

Carreau commented Oct 22, 2021

sgaist commented Oct 23, 2021

sgaist commented Oct 23, 2021

Carreau commented Oct 25, 2021

Carreau Oct 25, 2021

Carreau Oct 26, 2021

Carreau commented Oct 26, 2021

Carreau commented Oct 26, 2021

Nose cleanup first step #13206

Nose cleanup first step #13206

Conversation

sgaist commented Oct 20, 2021

Carreau 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

Carreau commented Oct 21, 2021

Carreau commented Oct 21, 2021

Kojoley commented Oct 21, 2021

Carreau commented Oct 21, 2021

Kojoley commented Oct 21, 2021

Carreau commented Oct 21, 2021

sgaist commented Oct 21, 2021

Carreau commented Oct 22, 2021

sgaist commented Oct 23, 2021

sgaist commented Oct 23, 2021

Carreau commented Oct 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Oct 26, 2021

Carreau commented Oct 26, 2021