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

Stop creating qute_test! #1637

Merged
merged 12 commits into from Jul 20, 2016
Merged

Stop creating qute_test! #1637

merged 12 commits into from Jul 20, 2016

Conversation

@rcorre
Copy link
Contributor

@rcorre rcorre commented Jul 10, 2016

~/.config/qute_test and ~/.local/share/qute_test kept showing up on disk.
They really mess with my tab completion, but took me a while to figure out it was these unittests, as I just kept assuming I forgot to delete them. 😄.

Also, the fixture to prevent creation of ~/.cache/qute_test wasn't working because usefixtures doesn't work on fixture, as a past version of you pointed out.


This change is Reviewable

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 10, 2016

Whoops! Seems like this makes TestCreatingDir.test_basedir[runtime] fail on Windows though, could you take a look?

@codecov-io
Copy link

@codecov-io codecov-io commented Jul 11, 2016

Current coverage is 81.40%

Merging #1637 into master will increase coverage by 0.01%

@@             master      #1637   diff @@
==========================================
  Files           110        110          
  Lines         16163      16163          
  Methods           0          0          
  Messages          0          0          
  Branches       2518       2518          
==========================================
+ Hits          13156      13158     +2   
+ Misses         2511       2510     -1   
+ Partials        496        495     -1   

Powered by Codecov. Last updated by 6f65973...7d36847

"""Test that system-wide path isn't used on linux if path not exist."""
args = types.SimpleNamespace(basedir=str(tmpdir))
standarddir.init(args)

This comment has been minimized.

@The-Compiler

The-Compiler Jul 11, 2016
Member

With the qtwebengine branch I also added a fake_args fixture so you can do fake_args.basedir = str(tmpdir) (similar to config_stub) and it also gets registered in objreg accordingly - would you mind rebasing and switching to that?

"""Test that system-wide path is not used on non-Linux OS."""
args = types.SimpleNamespace(basedir=str(tmpdir))
standarddir.init(args)

This comment has been minimized.

@rcorre rcorre force-pushed the rcorre:cut_test_clutter branch from bd2e6d7 to a18b1b1 Jul 12, 2016
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 12, 2016

Not sure why pylint is still failing as I did rerun it against the newest master... anyways, looks good locally.

edit: Err, no, it doesn't:

C:343, 0: Line too long (80/79) (line-too-long)

I'll fix it before merging though.

Also seems a new random Qt warning message popped up:

____________________ TestSessionDelete.test_deletion_error _____________________
tests/unit/misc/test_sessions.py:838: Failure: Qt messages with level WARNING or above emitted
----------------------------- Captured Qt messages -----------------------------
../gui/text/qfontengine_ft.cpp:QFontEngineFT::Glyph* QFontEngineFT::loadGlyph(QFontEngineFT::QGlyphSet*, uint, QFixed, QFontEngine::GlyphFormat, bool) const:881:
    QtWarningMsg: load glyph failed err=6 face=0x70855a0, glyph=260
py-3974.log.0

Time to whitelist another one, I guess.

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 12, 2016

hmm - after running all tests with this, I still see a ~/.config/qute_test being created.

The-Compiler added a commit that referenced this pull request Jul 12, 2016
@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 12, 2016

Argh, I broke it while trying to fix the windows build. Should be good now, as long as the windows build passes.

@rcorre rcorre force-pushed the rcorre:cut_test_clutter branch from 179102a to c1fa15f Jul 12, 2016
@rcorre rcorre force-pushed the rcorre:cut_test_clutter branch from c1fa15f to fdf3176 Jul 12, 2016
@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 12, 2016

Excuse the re-pushes, having trouble running pylint:

Traceback (most recent call last):
  File "/home/rcorre/projects/contrib/qutebrowser/.tox/pylint/bin/pylint", line 11, in <module>
    sys.exit(run_pylint())
  File "/home/rcorre/projects/contrib/qutebrowser/.tox/pylint/lib/python3.5/site-packages/pylint/__init__.py", line 23, in run_pylint
    Run(sys.argv[1:])
  File "/home/rcorre/projects/contrib/qutebrowser/.tox/pylint/lib/python3.5/site-packages/pylint/lint.py", line 1280, in __init__
    linter.load_plugin_modules(plugins)
  File "/home/rcorre/projects/contrib/qutebrowser/.tox/pylint/lib/python3.5/site-packages/pylint/lint.py", line 487, in load_plugin_modules
    module = modutils.load_module_from_name(modname)
  File "/home/rcorre/projects/contrib/qutebrowser/.tox/pylint/lib/python3.5/site-packages/astroid/modutils.py", line 186, in load_module_from_name
    return load_module_from_modpath(dotted_name.split('.'), path, use_sys)
  File "/home/rcorre/projects/contrib/qutebrowser/.tox/pylint/lib/python3.5/site-packages/astroid/modutils.py", line 228, in load_module_from_modpath
    mp_file, mp_filename, mp_desc = imp.find_module(part, path)
  File "/home/rcorre/projects/contrib/qutebrowser/.tox/pylint/lib/python3.5/imp.py", line 296, in find_module
    raise ImportError(_ERR_MSG.format(name), name=name)
ImportError: No module named 'bad_builtin'

I've run tox -r -e mkvenv, any idea what this is?

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 12, 2016

tox -r -e mkvenv only recreates .venv which is useful for testing stuff or running the newest qutebrowser from git.

pylint runs with a separate environment (.tox/pylint) - to run it you'd use tox -e pylint, so to recreate it (and thus install the newest pylint version) you'll need to do tox -e pylint -r. Unfortunately tox doesn't understand requirements.txt files, so it doesn't know when it needs to rebuild an enviroment because the dependency versions changed...

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 12, 2016

Ah, right. I remember you telling me this before :)

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 12, 2016

Hmm, looks like the test from #1639 is flaky - no idea why yet... Will take a look.

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 12, 2016

Turns out it just broke on Windows because backslashes are used for paths but also for escaping in the commandline 😉 Fixed it, rerunning the build now.

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 12, 2016

Hmm, I still see a ~/.config/qute_test though...

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 13, 2016

@The-Compiler really? After just running this test or the whole test suite? Maybe something else creates it...

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 13, 2016

Yup, you're right. Another test is guilty. I'll try to track it down

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 13, 2016

Alright, I think I've got the culprits:

-----
config:  tests/unit/browser/test_adblock.py tests/unit/config/test_configtypes.py tests/unit/config/test_configtypes_hypothesis.py tests/unit/config/test_config.py
-----
data:  tests/unit/browser/webkit/test_cookies.py tests/unit/browser/webkit/test_qt_javascript.py tests/unit/browser/test_tab.py

Assuming I did this right, my script ran them individually and checked for qute_test after each run.

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 13, 2016

For reference (mostly in case I need this again): here's the script

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 15, 2016

@The-Compiler I think I got em all this time. 80b3528 was a bit more complicated than the others. Let me know what you think of that solution. Is XDG respected on windows?

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 15, 2016

Apparently Appveyor is not in a condition to answer that question for me >.<

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Jul 16, 2016

Well, tox itself creates ~/.tox/distshare/qutebrowser-0.7.0.zip, even though it is exactly the same file as .tox/dist/qutebrowser-0.7.0.zip in the dev root directory.

Running tox -e py35 seems to start its own dbus daemon with dbus-launch --autolaunch - the daemon is running while the tests are running and ~/.dbus/ is left behind. I have a dbus running as systemd service, so maybe $DBUS_SESSION_BUS_ADDRESS could be reused?

Other than that, I didn't spot anything.

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 16, 2016

Oh, weird. pytestmark = pytest.mark.usefixtures('config_tmpdir') at the top of test_config causes it to leave around a LineParser that interferes with test_debug.

rcorre added 2 commits Jul 16, 2016
Using the config_tmdpir fixture across all tests in this module caused
a lingering LineParser to make test_debug fail.
I still don't know why, but scoping the config_tmpdir fixture to only
the test class that was creating ~/.config/qute_test fixes the issue,
and still prevents creation of a user tempdir.
This is another case (like test_qt_javascript) that needs redirection
of XDG_DATA_HOME to prevent Qt from creating ~/.share/local/qute_test.
@rcorre rcorre force-pushed the rcorre:cut_test_clutter branch from 3b93a4e to 7d36847 Jul 17, 2016
rcorre added 2 commits Jul 19, 2016
This ensures the environment is modified only for the test using the
fixture rather than for the whole test run.
Only use the fixture in the test class that tries to access the config
dir (TestFileAndUserStyleSheet) rather than the whole test.
@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 19, 2016

@The-Compiler I think this is good to go now. The two failures seem setup related

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 20, 2016

I've hit rebuild, but it might still take me a few days until I get to merging it 😉

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Jul 20, 2016

Sounds good, I may look into @lahwaacz's comment about ~/.pylint.d. I think I tried setting PYLINTHOME manually and it worked, so I just need to figure out how that fits into tox.ini

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 20, 2016

I had some time right now - already cleaned pylint and tox.

@The-Compiler The-Compiler merged commit 48dbf50 into qutebrowser:master Jul 20, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
The-Compiler added a commit that referenced this pull request Jul 20, 2016
The-Compiler added a commit that referenced this pull request Jul 20, 2016
This is kind uf unorthodox and the "distshare" setting seems to be
deprecated, but we don't get a ~/.tox this way.

See #1637
@rcorre rcorre deleted the rcorre:cut_test_clutter branch Jul 20, 2016
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jul 20, 2016

Merged, thanks!

Some commits on top of it:

  • pylint: Set persistent=n - c4fb43d
  • tox: Set distshare = {toxworkdir} - 5f2dc53
  • Rename shadowed tmpdir variable - a1c4e6e
@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Aug 28, 2016

Now it seems that ~/.qute_test/ and ~/.QtWebEngineProcess/ are left behind after the webengine part of the tests. Unfortunately the whole test suite does not pass for me due to #1819, can somebody confirm that these directories stay after running tox -e py35?

Also there is still the problem with ~/.dbus/, which is created about 3 minutes before the previous two dirs, so it seems unrelated to webengine. Can anything be done about this or is it a lost cause?

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Aug 28, 2016

@lahwaacz I think at least the ~/.qute_test one was fixed in Qt, not sure about ~/.QtWebEngineProcess.

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Aug 28, 2016

@The-Compiler IIRC one of the .qute_test instances was created internally by QtWebKit andt I had to fix it by setting and environment variable, so it makes sense that it might come back with the WebEngine tests

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Aug 29, 2016

I don't think they were ever created with QtWebKit - I'm only aware of QTBUG-54258.

Though I'd expect that to be prevented via the redirect_xdg_data fixture @rcorre added - which tests are you running exactly, @lahwaacz?

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Aug 29, 2016

I was running tox -e py35, but not all of it (#1819). I narrowed it down to tests/unit/browser/test_tab.py::test_tab[QWebEngineView], which is also the test that fails for me in xvfb, but ~/.qute_test and ~/.QtWebEngineProcess are created even with --no-xvfb.

@rcorre
Copy link
Contributor Author

@rcorre rcorre commented Aug 29, 2016

Whoops, I misread. ~/.qute_test was never created -- it was ~/.local/share/qute_test and ~/.config/qute_test. Do you have $XDG_DATA_HOME or $XDG_CONFIG_HOME set?

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Aug 29, 2016

Yes, they point to the usual paths ~/.local/share and ~/.config respectively.

The-Compiler added a commit that referenced this pull request Sep 12, 2016
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Sep 12, 2016

Seems like QtWebEngine happily writes stuff to the homedir without respecting any XDG_* vars...

I also redirected that in e6680c3 and opened QTBUG-55946 about it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants