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

automatic tests for TUI (ncurses) #251

Merged
merged 16 commits into from
Oct 27, 2020
Merged

automatic tests for TUI (ncurses) #251

merged 16 commits into from
Oct 27, 2020

Conversation

mvidner
Copy link
Member

@mvidner mvidner commented Oct 6, 2020

@@ -12,3 +17,8 @@ endforeach(test)

ADD_TEST("integration" ruby ${CMAKE_CURRENT_SOURCE_DIR}/integration/run.rb)
ADD_TEST("translations" rspec --format doc ${CMAKE_CURRENT_SOURCE_DIR}/integration/translations_spec.rb)

file(GLOB libyui_tests "libyui/*.test")
Copy link
Member

Choose a reason for hiding this comment

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

Well, tests are in tmux which I do not know much, but fine for me. I have other issues:

  1. why it lives in yast2-ruby-bindings? it should be in libyui itself or libyui-bindings if you really want to use ruby and not write it as example program in C++. Do you really expect that libyui contributors search for it here and also run it on their magei/buntu/etc? Ideally it should be make && make check in libyui itself

  2. It tests too late. What happens here? It needs libyui, its bindings, then ycp-ui-bindings, then yast2-core and then yast2-ruby-bindings and then you can see what failed. To be honest I think it will be in comparable speed to get result from openqa itself. It is too slow for unit testing and looks more like full integration testing, where openqa is a better as it test more and has more tests.

@mvidner
Copy link
Member Author

mvidner commented Oct 7, 2020

@jreidinger

  1. why it lives in yast2-ruby-bindings? it should be in libyui itself or
    libyui-bindings if you really want to use ruby and not write it as example
    program in C++. Do you really expect that libyui contributors search for it
    here and also run it on their magei/buntu/etc? Ideally it should be make &&
    make check in libyui itself

The motivation for this is: we make some changes to libyui (add sort keys, add
menus, add nested table rows) and it breaks in unexpected places. We need
tests.

The deciding factor is: who writes the tests and in what language? My answer is: the yast people, in Ruby.

We have already tried writing tests in C++, see
https://github.com/libyui/libyui-test , it hasn't taken off.

https://github.com/libyui/libyui is an abstract base, good only for elementary
unit tests. Sorry, cannot work for the tests that we need.

https://github.com/libyui/libyui-bindings is IMHO immature and unused. And
https://github.com/yast/yast-ycp-ui-bindings/tree/master/examples already has
the canonical collection of examples which we can directly use or adapt.

Libyui contributors are, for now, a secondary concern. We first need to enable
testing for the core team.

  1. It tests too late. What happens here? It needs libyui, its bindings, then
    ycp-ui-bindings, then yast2-core and then yast2-ruby-bindings and then you
    can see what failed. To be honest I think it will be in comparable speed to
    get result from openqa itself. It is too slow for unit testing and looks
    more like full integration testing, where openqa is a better as it test
    more and has more tests.

Our team simply does not write openQA tests. IMHO it's YaST tests or no tests.

And no, this can still be run by the developer locally as they modify libyui. Installing a handful of yast packages on top of libyui is still much less of a burden than waiting for the massive openQA machinery.

@jreidinger
Copy link
Member

@mvidner still it does not much convince to me to be the tests that helps with development. Another idea, if tests are for ncurses only, then I suggest to place it in libyui-ncurses and make it mandatory ( so part of rpm spec build and also travis ). so we immediately see in pr if it breaks tests or not. And similar way we can use some qt testing framework for libyui-qt. I think it should live with code otherwise it become obsolete, even here in ruby-bindings, as it does not change often and you catch it too late.
For me reason why libyui-test failed is that it is not part of code. If tests are run as part of travis and ideally rpm build, then it will be maintained and provide early feedback if something goes south.

@ancorgs
Copy link
Contributor

ancorgs commented Oct 7, 2020

@mvidner still it does not much convince to me to be the tests that helps with development. Another idea, if tests are for ncurses only, then I suggest to place it in libyui-ncurses and make it mandatory ( so part of rpm spec build and also travis ). so we immediately see in pr if it breaks tests or not.

It's true that it would be good to have the tests triggered for every pull request opened against libyui-ncurses (adding the tests to the rpm spec build doesn't sound so urgent to me). If such pull request needs adaptation to the tests, having them separated can indeed be a problem.

On the other hand, previous @mvidner's arguments have convinced me about the convenience of having the tests in this repository. For me, ensuring the tests are seen, understood and maintained by the YaST Team (who are mainly Ruby developers that could easily come up with tests by basically copy/pasting real pieces of YaST) is the most relevant criteria here.

using tmux-uitest.sh
FIXME: resolve where tmux-uitest.sh belongs, give it some header and docs
Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

I'm getting this test errors when running rake osc:build locally:

[   17s] The following tests FAILED:
[   17s] 	 25 - integration (Failed)
[   17s] 	 27 - /home/abuild/rpmbuild/BUILD/yast2-ruby-bindings-4.3.5/tests/libyui/table_sort_wrong_cell_1165388.test (Failed)
[   17s] Errors while running CTest
[   17s] make: *** [Makefile:137: test] Error 8

How can I get more details about the failures?

package/yast2-ruby-bindings.spec Outdated Show resolved Hide resolved
tests/libyui/table_sort_wrong_cell_1165388.rb Outdated Show resolved Hide resolved
tests/libyui/table_sort_wrong_cell_1165388.test Outdated Show resolved Hide resolved
tests/libyui/tmux-uitest.sh Outdated Show resolved Hide resolved
@mvidner
Copy link
Member Author

mvidner commented Oct 16, 2020

Just when I thought I can finally finish this it turns out that latest libyui broke the single test that I have here ☹️

# -x width -y height,
# -d detached
# FIXME: sleep to be able to see errors when running $1
tmux new-session -s "$SESSION" -x 80 -y 24 -d sh -c "$1; sleep 9999"
Copy link
Member Author

Choose a reason for hiding this comment

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

@lslezak you asked "How can I get more details about the failures?"

This sleep lets us capture the screen after a command fails and exits.

Tmux has a remain-on-exit option but I haven't figured out how to enable it yet :-(

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha! Totally intuitive for a beginner... NOT

tmux set-hook -g session-created 'set remain-on-exit on' \; new-session sh -c ...

Found in tmux/tmux#787

The example UI scripts that we are starting to use for testing are marked
as docs in the RPM and the container images exclude them.
Explicitly install them.
ex.run

if @tui.has_session?
@tui.kill_session
Copy link
Member

Choose a reason for hiding this comment

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

why two methods? I would have just one like kill_session that internally check if session exists.

Copy link
Member

Choose a reason for hiding this comment

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

NP: Maybe we should print a warning on STDERR in this case, each test tries to finish cleanly (by pressing "Quit" or "Close" button), if the test is still running then something went wrong.

describe "Menu Item" do
bug = "1177760"
around(:each) do |ex|
@base = "menu_hotkeys_#{bug}"
Copy link
Member

Choose a reason for hiding this comment

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

where is it used?

Copy link
Member Author

Choose a reason for hiding this comment

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

in the screenshot filenames: @tui.capture_pane_to("#{@base}-1-initial")


yast_ncurses = "#{__dir__}/yast_ncurses"
example_dir = "/usr/share/doc/packages/yast2-ycp-ui-bindings/examples"
@tui = TmuxTui.new_session "#{yast_ncurses} #{example_dir}/MenuBar1.rb"
Copy link
Member

Choose a reason for hiding this comment

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

I expect this whole creation need to be redone in each test. So why not simply have helper in TmuxTui and have something like @tui = TmuxTui.testing_client("MenuBar1.rb") instead of those three lines?

"-x", @x.to_s,
"-y", @y.to_s,
*(@detach ? ["-d"] : [] ),
"sh", "-c", "#{@shell_command}; sleep 9999"
Copy link
Member

Choose a reason for hiding this comment

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

is that long sleep still needed? I think you can do in rspec specific expect name like expect(@tui).to include_in_pane("Last Event") and have own specific error handler there if you need to somehow capture it.

@@ -0,0 +1,36 @@
require_relative "rspec_tmux_tui"

Copy link
Member

Choose a reason for hiding this comment

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

NP: It would be nice to have a full bug URL here, just having a bug number might not be enough for people not familiar with libyui or SUSE.

ex.run

if @tui.has_session?
@tui.kill_session
Copy link
Member

Choose a reason for hiding this comment

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

NP: Maybe we should print a warning on STDERR in this case, each test tries to finish cleanly (by pressing "Quit" or "Close" button), if the test is still running then something went wrong.

txt = capture_pane(color: false)
esc = capture_pane(color: true)
File.write("#{filename}.out.txt", txt)
File.write("#{filename}.out.esc", esc)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer .raw suffix so it's clear that it's the original unprocessed screen dump.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, as I understand it, there is no such thing as a raw terminal dump, unless we're talking about the linux console devices (/dev/vcsa7 having colors for /dev/vcs7). I feel .esc is more specific.

end

def has_session?
system "tmux", "has-session", "-t", session_name
Copy link
Member

Choose a reason for hiding this comment

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

Um, because of that sleep 9999 the session will be always present, or am I wrong ❓


bug = "1177145"
it "ChangeWidget(_, :CurrentItem) activates the correct line, boo##{bug}" do
skip "not fixed yet"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe marking as pending would be better than skipping completely?

https://relishapp.com/rspec/rspec-core/v/3-8/docs/pending-and-skipped-examples

"remain-on-exit" is useful if shell_command may unexpectedly fail quickly.
In that case we can still capture the pane and read the error messages.
@mvidner mvidner merged commit 6e5a650 into master Oct 27, 2020
@mvidner mvidner deleted the libyui-tests branch October 27, 2020 11:50
@yast-bot
Copy link
Contributor

❌ Internal Jenkins job #21 failed

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.

5 participants