Skip to content

Reenable terminal tests on CI #5018

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Reenable terminal tests on CI #5018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f48ea3c
Reproduce CI failure in fixing TODO
Blaisorblade Aug 25, 2018
953c384
Allow dumb terminals
Blaisorblade Aug 25, 2018
7e65230
Drop fixed TODO
Blaisorblade Aug 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .drone.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ pipeline:
commands:
- cp -R . /tmp/4/ && cd /tmp/4/
- ./project/scripts/sbt sbt-dotty/scripted
when:
# sbt scripted tests are slow and only run on nightly or deployment
event: [ tag, deployment ]
# when:
# # sbt scripted tests are slow and only run on nightly or deployment
# event: [ tag, deployment ]
Copy link
Contributor

Choose a reason for hiding this comment

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

This need to be reverted


# DOCUMENTATION:
documentation:
Expand Down
1 change: 0 additions & 1 deletion compiler/src/dotty/tools/repl/JLineTerminal.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ final class JLineTerminal extends java.io.Closeable {
// Logger.getLogger("org.jline").setLevel(Level.FINEST)

private val terminal = TerminalBuilder.builder()
.dumb(false) // fail early if not able to create a terminal
Copy link
Contributor

Choose a reason for hiding this comment

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

I am against enabling this by default. Last time I tried, the dumb terminal was very limited and I don't want to expose it to users. E.g.

  • no syntax highlighting
  • no history
  • no auto-completion
  • no multi-line editing

You can experiment with dumb terminal with these changes:

diff --git a/compiler/src/dotty/tools/repl/JLineTerminal.scala b/compiler/src/dotty/tools/repl/JLineTerminal.scala
index 22e44e4a5..9c33a7b87 100644
--- a/compiler/src/dotty/tools/repl/JLineTerminal.scala
+++ b/compiler/src/dotty/tools/repl/JLineTerminal.scala
@@ -17,9 +17,12 @@ final class JLineTerminal extends java.io.Closeable {
   // import java.util.logging.{Logger, Level}
   // Logger.getLogger("org.jline").setLevel(Level.FINEST)
 
-  private val terminal = TerminalBuilder.builder()
-    .dumb(false) // fail early if not able to create a terminal
-    .build()
+  // private val terminal = TerminalBuilder.builder()
+  //   .dumb(false) // fail early if not able to create a terminal
+  //   .build()
+  import java.lang.System
+  private val terminal =
+    new org.jline.terminal.impl.DumbTerminal(System.in, System.out)
   private val history = new DefaultHistory
 
   private def blue(str: String) = Console.BLUE + str + Console.RESET

I would be Ok to make this configurable in order to use dumb terminal in tests. However, we clearly need to document what can be tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I just wanted to enable it as fallback if there is no alternative. Even a dumb REPL seems better than no REPL at all? Tho we should probably warn the user to fix their terminal.

Anyway, supporting dumb terminals for users is a separate question.

.build()
private val history = new DefaultHistory

Expand Down
3 changes: 1 addition & 2 deletions sbt-dotty/sbt-test/sbt-dotty/example-project/test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
> run
> 'set initialCommands := "1 + 1" '
# FIXME: does not work on the CI
#> console
> console
Copy link
Contributor

Choose a reason for hiding this comment

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

A more extensive REPL test would be nice. I don't know to what extent we can test the REPL with scripted tests. E.g. can we test something like #3007?

Copy link
Contributor Author

@Blaisorblade Blaisorblade Aug 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, this already tests that starting the console succeeds, which seems why @SzymonPajzert was interested for #4933.

If we want to connect something to the actual terminal, give commands and wait for responses, one could use expect-like programs.
If we bypass the terminal logic, it seems we'd have to add a consoleTest script command which combines console with ReplTest: it'd be a ConsoleInterface creating, instead of ReplDriver, a ScriptedTests on the script. EDIT: but that, and interfacing to sbt for that, seems much more than I plan for this PR or I can take on.