Expand the serial terminal console documentation #1586

richiejp · 2018-02-19T15:45:41Z

https://progress.opensuse.org/issues/31777

richiejp · 2018-02-19T16:03:22Z

pevik · 2018-02-19T18:01:13Z

LGTM. Thanks Richie to extend docs!
Tiny note: could you name them instead of using first and second?

asmorodskyi · 2018-02-19T19:53:18Z

docs/WritingTests.asciidoc

+instead of being interpreted by a terminal emulator which then renders the
+text.
+
+However the advantages of using the serial terminal usually outweigh the


Very discussable statement , would better remove it. It's up to openQA consumer to decide which way is better

asmorodskyi · 2018-02-19T20:02:22Z

docs/WritingTests.asciidoc

+
+[source,perl]
+--------------------------------------------------------------------------------
+if (select_virtio_console()) {


it's a big question for me to document function which is in testing repository . From openQA perspective os-autoinst-distri-opensuse it is just another testing repository. How guys from Fedora would follow this ?
I am agree that select_virtio_console is needed function but to insert it to documentation need to move to os-autoinst

Or at least you need to point where this function is declared and explain that it's only related to os-autoinst-distri-opensuse

I prefer the latter because most distributions should have something similar and it is nice to have complete documentation in one place.

asmorodskyi · 2018-02-19T20:05:04Z

docs/WritingTests.asciidoc

+
+1) Wait for the terminal prompt to appear.
+2) Send your command
+3) Wait for your command text to be echoed by the shell (inf applicable)


asmorodskyi · 2018-02-19T20:09:09Z

docs/WritingTests.asciidoc

+
+if (is_serial_terminal) {
+        script_run($klog_stamp);
+        wait_serial(serial_term_prompt(), undef, 0, no_regex => 1);


can you elaborate more about serial_term_prompt() ?

actually it make sense to explain other parameters in wait_serial and why you using them. It will give some insights about how serial terminal actually working

but maybe I am asking too much for single PR ... feel free to ignore this one

Agree :-). I mean it's great to have comments but on the other hand perfect is the enemy of good :-).

Thank you both for your care.

I think it is perfectly valid criticism :-)

asmorodskyi · 2018-02-19T20:13:40Z

docs/WritingTests.asciidoc

+
+Then the test script enters our command with +type_string+ and waits for the
+command's text to be echoed back by the shell. Shell's usually echo the
+characters sent to them so that the user can see what they have typed. However


and when it is not the case and why ?

this one is just like ongoing question not directly related to PR :)

Ok, well I have explained a bit more anyway in the docs, but a full explanation would not be relevant because it is only likely to be turned off deliberately to simplify processing SUT output which I don't think is a good idea.

Another situation where it might be turned off is if your local terminal emulator displays your keypresses. In which case the remote TTY/PTY does not need to. Another is if you are using a very stupid terminal on an embedded device which does not echo the characters. Also if you are using a very slow serial connection, the echo might interfere with the characters you are trying to send, so it is best to turn it off. None of this is very likely when you are using a Linux capable device, but I think it is helpful knowledge for understanding how terminals work.

asmorodskyi · 2018-02-19T20:15:45Z

docs/WritingTests.asciidoc

+script_run+.
+
+We choose to wait for the prompt just before sending a command, rather than
+after it, so that step 5 can be deferred to a later time. In theory this


you not provide numbering so it is hard to figure out what is step 5

asmorodskyi · 2018-02-20T20:39:12Z

docs/WritingTests.asciidoc

+my $cmd_text   = qq($test->{command}; echo "$fin_msg\$?");
+my $klog_stamp = "echo 'OpenQA::run_ltp.pm: Starting $test->{name}' > /dev/$serialdev";
+
+# More variables and other shit


could you please use more polite word here :)

asmorodskyi · 2018-02-20T20:43:15Z

LGTM . Well done @richiejp !

codecov · 2018-02-21T09:36:19Z

Codecov Report

Merging #1586 into master will decrease coverage by 24.59%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1586      +/-   ##
=========================================
- Coverage   82.59%     58%   -24.6%     
=========================================
  Files         121     106      -15     
  Lines        9037    8255     -782     
=========================================
- Hits         7464    4788    -2676     
- Misses       1573    3467    +1894

Impacted Files	Coverage Δ
lib/OpenQA/File.pm	`8.14% <0%> (-91.86%)`	⬇️
lib/db_profiler.pm	`12% <0%> (-88%)`	⬇️
lib/OpenQA/Client/Upload.pm	`12.19% <0%> (-87.81%)`	⬇️
lib/OpenQA/Resource/Locks.pm	`9.33% <0%> (-84%)`	⬇️
lib/OpenQA/Parser.pm	`17.92% <0%> (-82.08%)`	⬇️
lib/OpenQA/Client/Handler.pm	`18.18% <0%> (-81.82%)`	⬇️
lib/OpenQA/Worker/Cache.pm	`6.36% <0%> (-80%)`	⬇️
lib/OpenQA/Parser/Results.pm	`21.73% <0%> (-78.27%)`	⬇️
lib/OpenQA/Parser/Result.pm	`22.85% <0%> (-77.15%)`	⬇️
lib/OpenQA/Schema/Result/Screenshots.pm	`7.69% <0%> (-67%)`	⬇️
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7472fdf...205956a. Read the comment docs.

richiejp · 2018-02-21T09:37:10Z

Thanks for review @asmorodskyi and @pevik

okurz · 2018-02-26T21:35:53Z

I feel weird to critique a native speaker but shouldn't the commit/PR say "extend" rather than "expand"?

richiejp · 2018-02-27T09:59:42Z

"extend" is a synonym for "expand" in ordinary English (i.e. non-technical language). "elaborate" is probably more correct than both of them. However I'm not a grammar expert.

SergioAtSUSE

Thanks for the documentation!

SergioAtSUSE · 2018-03-02T15:59:58Z

docs/WritingTests.asciidoc

+usually output to.
+
+When switching to a virtio based serial terminal, +wait_serial+ will then read
+from the virtio serial port instead. However the emulated serial port still


As spoken. Perhaps you can find better names to identify each port. ;)

SergioAtSUSE · 2018-03-02T16:02:08Z

docs/WritingTests.asciidoc

+
+# More variables and other stuff
+
+if (is_serial_terminal) {


Here a reminder about our conversation about the differences between how a session is initiated using virtio and using VNC not. (if I understood well)

SergioAtSUSE · 2018-03-02T16:04:21Z

docs/WritingTests.asciidoc

+        wait_serial($cmd_text, undef, 0, no_regex => 1);	    #Step 3
+        type_string("\n");     	      	 	     		    #Step 4
+} else {
+        # The slow path


Reminder to improve this block so it is easier to understand what is meant by slow path. Like your suggestion about putting the "slow commands".

coolo · 2018-03-03T08:31:38Z

I'll merge this as first round - improvements can go into a second

commit 9cbbef7 Author: Richard Palethorpe (rpalethorpe) <richiejp@f-m.fm> AuthorDate: Sat Mar 3 09:31:41 2018 +0100 Commit: Stephan Kulow <stephan@kulow.org> CommitDate: Sat Mar 3 09:31:41 2018 +0100 Expand the serial terminal console documentation (#1586)

asmorodskyi reviewed Feb 19, 2018

View reviewed changes

richiejp force-pushed the serial_terminal_docs branch from 978be65 to 7fffeec Compare February 20, 2018 12:02

richiejp changed the title ~~[WIP] Expand the serial terminal console documentation~~ Expand the serial terminal console documentation Feb 20, 2018

richiejp force-pushed the serial_terminal_docs branch from 7fffeec to 2c98c7e Compare February 20, 2018 12:40

asmorodskyi reviewed Feb 20, 2018

View reviewed changes

Expand the serial terminal console documentation

205956a

richiejp force-pushed the serial_terminal_docs branch from 2c98c7e to 205956a Compare February 21, 2018 09:36

SergioAtSUSE reviewed Mar 2, 2018

View reviewed changes

SergioAtSUSE mentioned this pull request Mar 2, 2018

Use normal root-console to allow sending ctrl-c os-autoinst/os-autoinst-distri-opensuse#4523

Merged

coolo merged commit 9cbbef7 into os-autoinst:master Mar 3, 2018

richiejp mentioned this pull request Mar 7, 2018

docs: Clarify some serial terminal terminology and other elaboration #1596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand the serial terminal console documentation #1586

Expand the serial terminal console documentation #1586

richiejp commented Feb 19, 2018

richiejp commented Feb 19, 2018

pevik commented Feb 19, 2018

asmorodskyi Feb 19, 2018

asmorodskyi Feb 19, 2018 •

edited

Loading

asmorodskyi Feb 19, 2018

richiejp Feb 20, 2018

asmorodskyi Feb 19, 2018

asmorodskyi Feb 19, 2018

asmorodskyi Feb 19, 2018

asmorodskyi Feb 20, 2018

pevik Feb 20, 2018

richiejp Feb 20, 2018

asmorodskyi Feb 19, 2018

asmorodskyi Feb 20, 2018

richiejp Feb 20, 2018

asmorodskyi Feb 19, 2018

asmorodskyi Feb 20, 2018

asmorodskyi commented Feb 20, 2018

codecov bot commented Feb 21, 2018

richiejp commented Feb 21, 2018

okurz commented Feb 26, 2018

richiejp commented Feb 27, 2018

SergioAtSUSE left a comment

SergioAtSUSE Mar 2, 2018

SergioAtSUSE Mar 2, 2018

SergioAtSUSE Mar 2, 2018

coolo commented Mar 3, 2018

Expand the serial terminal console documentation #1586

Expand the serial terminal console documentation #1586

Conversation

richiejp commented Feb 19, 2018

richiejp commented Feb 19, 2018

pevik commented Feb 19, 2018

Choose a reason for hiding this comment

asmorodskyi Feb 19, 2018 • edited Loading

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

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

asmorodskyi commented Feb 20, 2018

codecov bot commented Feb 21, 2018

Codecov Report

richiejp commented Feb 21, 2018

okurz commented Feb 26, 2018

richiejp commented Feb 27, 2018

SergioAtSUSE 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

coolo commented Mar 3, 2018

asmorodskyi Feb 19, 2018 •

edited

Loading