Reactivate integration tests #85
Conversation
knixeur
force-pushed
the
luaposix
branch
2 times, most recently
from
14fadda to
08c4568
Compare
|
This is marked as WIP, what's missing here? I think integration tests might be a good idea if it isn't a lot of effort to finish this off. |
|
I think it's ready, it works like it did before. I think I need someone to test it too and then finish the items in the checklist (luarocks + docs) |
knixeur
force-pushed
the
luaposix
branch
2 times, most recently
from
8dbec06 to
a58987a
Compare
|
very cool to replace that XDummy script with a configuration 👍 great work here |
|
I guess on travis we might use https://docs.travis-ci.com/user/gui-and-headless-browsers/#using-xvfb-to-run-tests-that-require-a-gui - but then perhaps we don't need to start our own X11 server in the script? |
knixeur
force-pushed
the
luaposix
branch
3 times, most recently
from
62a372c to
497df4b
Compare
knixeur
changed the title
| @@ -22,8 +67,8 @@ print('Updating layout when there is 2 screens present') | |||
| -- remove one, and then leave checking that one was removed up to the next test. | |||
| mod_xrandr.screenlayoutupdated() | |||
|
|
|||
| if notioncore.find_screen_id(0):mx_count() ~= 1 then | |||
| return "After updating screen 0 should have 1 workspaces instead of " .. | |||
| if notioncore.find_screen_id(0):mx_count() ~= 2 then | |||
There was a problem hiding this comment.
so updating the screen layout, with 2 screens connected, 'spontaneously' another workspace appears? That doesn't look right - might be a problem with the test setup of course, I'll try and see if I can explain this one
There was a problem hiding this comment.
I think I've seen this on my setup, if I attach a second screen, it comes with two workspaces.
| mod_xrandr.screenlayoutupdated() | ||
|
|
||
| if notioncore.find_screen_id(1) then | ||
| return "New number of screens should be 1, found ", notioncore.find_screen_id(1):name() | ||
| if notioncore.find_screen_id(1) ~= nil then |
| end | ||
|
|
||
| if notioncore.find_screen_id(0):mx_count() ~= 2 then | ||
| return "Remaining screen should have 2 workspaces instead of " .. notioncore.find_screen_id(0):mx_count() .. | ||
| if notioncore.find_screen_id(0):mx_count() ~= 3 then |
There was a problem hiding this comment.
Seems surprising - but perhaps a side-effect of the previous test?
There was a problem hiding this comment.
Could be, I should do some more testing here confirmed it's a side effect of test 01
There was a problem hiding this comment.
Test 02 of xinerama suffers from the same issue, if test 01 is not ran it fails. I wonder when this started to occur since I didn't modify that one.
| if notioncore.find_screen_id(1):mx_nth(0):name() ~= 'WGroupWS<1>' then | ||
| return "Second workspace not correctly returned to second screen" | ||
| if notioncore.find_screen_id(0):mx_nth(1):name() ~= 'WGroupWS<1>' then | ||
| return "Second workspace correctly kept on first screen" |
There was a problem hiding this comment.
I think the intent here was the test would return either "ok" or an error
There was a problem hiding this comment.
I guess something like "Second workspace not kept in first screen" if that's the desired behaviour
Due to the improvements on notionflux where print() strings are also sent to output, we now check if the test's output buffer ends with "ok" instead of expecting it to be the only output.
I don't know why it's not finding mod_xrandr_mock now, I'd appreciate some help.
Probably either notion/notionflux behaviour changed while the tests weren't run or something broke on the tests. I'd like some feedback here.
|
So... do we merge this? |
There was a problem hiding this comment.
I would love to merge this, but I'm concerned about the changes that had to be made to the tests themselves (e.g. https://github.com/raboof/notion/pull/85/files#diff-03f7dac51cb8dd97abcd62197711cb64R70).
So while the tests are 'succeeding', they are only doing so after changing the expectation in the testcase - casting doubt on whether the implementation and the test are actually OK. They're not too easy to read unfortunately... I think we should look into that better before merging.
|
I agree with @raboof here, I think we should really understand the tests before merging this. |
Added check for lua-posix module, it can be installed with distribution's package manager or lua rocks
Rebase against LGPL
Explain on how to install Lua Rocks
Autodetect (which?) Xorg binary directory
Explain that xorg dummy driver is required for integration testing