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

PR: Fix scrolling issues #360

Merged
merged 5 commits into from
Aug 9, 2019
Merged

Conversation

impact27
Copy link
Contributor

@impact27 impact27 commented Aug 8, 2019

Fixes #351
Before:
before

After:
after

@impact27
Copy link
Contributor Author

impact27 commented Aug 8, 2019

I still need some tests. I want to check the position of w._control.verticalScrollBar().value().

  • Running a single command with enough prompts that it could scroll down to check it stays in place
import time
for i in range(10000):
    print(i)
    time.sleep(.1)
  • run this loop and check:
    • The verticalScrollBar scrolls with the output
    • If I move up, the verticalScrollBar stops following the output
    • if I move back down, the verticalScrollBar follows the output again

I am unsure how to do that, are there tests where a console is actually opened?

@ccordoba12
Copy link
Collaborator

I am unsure how to do that, are there tests where a console is actually opened?

Not here, but you can find how to do that in this file:

https://github.com/spyder-ide/spyder-kernels/blob/9d62e2e891fb133b086ba42d6f8a3e0c21277b45/spyder_kernels/ipdb/tests/test_matplotlib.py

@impact27
Copy link
Contributor Author

impact27 commented Aug 8, 2019

I added test but I don't know exactly how to run pytest while the rest is in unittest. Could somebody fix the tests? They all run individually on my machine.

Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@impact27, great job at creating a test for this! I left you some comments to simplify it by using the qtbot fixture, which is part of the pytest-qt package.

Please add it to this line

https://github.com/jupyter/qtconsole/blob/master/.travis.yml#L8

to be able to use it in our tests.

qtconsole/tests/test_console_widget.py Outdated Show resolved Hide resolved
qtconsole/tests/test_console_widget.py Outdated Show resolved Hide resolved
qtconsole/tests/test_console_widget.py Outdated Show resolved Hide resolved
qtconsole/tests/test_console_widget.py Outdated Show resolved Hide resolved
qtconsole/tests/test_console_widget.py Outdated Show resolved Hide resolved
qtconsole/tests/test_console_widget.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Collaborator

I added test but I don't know exactly how to run pytest while the rest is in unittest

I don't understand what you mean by this. Pytest runs all our tests, as can be seen here:

https://github.com/jupyter/qtconsole/blob/master/.travis.yml#L21

@impact27
Copy link
Contributor Author

impact27 commented Aug 8, 2019

I had to run the new tests first for some reason, by renaming the test file, but at least it works. I would be grateful if someone can explain why I need to do that.

@ccordoba12 ccordoba12 added this to the 4.5.3 milestone Aug 8, 2019
Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be grateful if someone can explain why I need to do that.

No idea. Perhaps moving everything to use pytest-qt would fix that problem.

However, if you want to run your new tests first, please use pytest-ordering and mark them with the @pytest.mark.first decorator. That will make them run at the beginning of our test suite.

qtconsole/tests/test_aaconsole_widget.py Outdated Show resolved Hide resolved
rename test file so it is executed first and the test pass


set argv to []


fix
@ccordoba12
Copy link
Collaborator

I think this is almost ready. One last comment: is it still necessary to rename test_console_widget? I thought that by using pytest-ordering that wouldn't be necessary.

@impact27
Copy link
Contributor Author

impact27 commented Aug 9, 2019

I didn't manage to make it work, I think it is only reordering tests in a same file.

@ccordoba12
Copy link
Collaborator

Then there's no need to use pytest-ordering, right?

I also think it's better to rename test_console_widget.py to 00_test_console_widget.py, to avoid future files to cause failures.

@impact27
Copy link
Contributor Author

impact27 commented Aug 9, 2019

test_00_console_widget.py so pytest still runs it.

@ccordoba12
Copy link
Collaborator

Yep, right.

Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @impact27!

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.

Prompt automatically scrolling down to the bottom of the window on execution
3 participants