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

Test start #2718

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions nyxt.asd
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ The renderer is configured from NYXT_RENDERER or `*nyxt-renderer*'."))
:targets (:package :nyxt/tests)
:serial t
:components ((:file "tests/package")
(:file "tests/offline/start")
(:file "tests/offline/define-configuration")
(:file "tests/offline/global-history")
(:file "tests/offline/user-script-parsing")
Expand Down
72 changes: 37 additions & 35 deletions source/start.lisp
Original file line number Diff line number Diff line change
Expand Up @@ -163,40 +163,41 @@ Without --remote, it also disables socket use."))))

(define-command quit (&optional (code 0))
"Quit Nyxt."
(when (slot-value *browser* 'ready-p)
(progn
(hooks:run-hook (before-exit-hook *browser*))
;; Unready browser:
;; - after the hook, so that on hook error the browser is still ready;
;; - before the rest, so to avoid nested `quit' calls.
(setf (slot-value *browser* 'ready-p) nil)
(setf (slot-value *browser* 'exit-code) code)
(dolist (window (window-list))
(window-delete window :force-p (/= 0 code)))
(when (socket-thread *browser*)
(destroy-thread* (socket-thread *browser*))
;; Warning: Don't attempt to remove socket-path if socket-thread was not
;; running or we risk remove an unrelated file.
(let ((socket (files:expand *socket-file*)))
(when (uiop:file-exists-p socket)
(log:info "Deleting socket ~s." socket)
(uiop:delete-file-if-exists socket))))
(mapc #'destroy-thread* (non-terminating-threads *browser*))
(ffi-kill-browser *browser*)
;; Reset global state.
(setf *browser* nil
*options* nil)
(uninstall *renderer*)
;; On FreeBSD this may cause freeze. Also we have to pass
;; FINISH-OUTPUT = NIL in FFI-INITIALIZE.
#-freebsd
(unless *run-from-repl-p*
(run-thread "force-quitter"
;; Force-quit in case `ffi-kill-browser' hangs. Must be run in a
;; separate thread because the renderer loop is waiting for this
;; function to finish.
(sleep 1)
(uiop:quit code nil))))))
(if (not (and *browser* (slot-value *browser* 'ready-p)))
(progn (echo "There's no browser instance to kill.") nil)
(progn
(hooks:run-hook (before-exit-hook *browser*))
;; Unready browser:
;; - after the hook, so that on hook error the browser is still ready;
;; - before the rest, so to avoid nested `quit' calls.
(setf (slot-value *browser* 'ready-p) nil)
(setf (slot-value *browser* 'exit-code) code)
(dolist (window (window-list))
(window-delete window :force-p (/= 0 code)))
(when (socket-thread *browser*)
(destroy-thread* (socket-thread *browser*))
;; Warning: Don't attempt to remove socket-path if socket-thread was not
;; running or we risk remove an unrelated file.
(let ((socket (files:expand *socket-file*)))
(when (uiop:file-exists-p socket)
(log:info "Deleting socket ~s." socket)
(uiop:delete-file-if-exists socket))))
(mapc #'destroy-thread* (non-terminating-threads *browser*))
(ffi-kill-browser *browser*)
;; Reset global state.
(setf *browser* nil
*options* nil)
(uninstall *renderer*)
;; On FreeBSD this may cause freeze. Also we have to pass
;; FINISH-OUTPUT = NIL in FFI-INITIALIZE.
#-freebsd
(unless *run-from-repl-p*
(run-thread "force-quitter"
;; Force-quit in case `ffi-kill-browser' hangs. Must be run in a
;; separate thread because the renderer loop is waiting for this
;; function to finish.
(sleep 1)
(uiop:quit code nil))))))

(define-command quit-after-clearing-session (&key confirmation-p) ; TODO: Rename?
"Close all buffers then quit Nyxt."
Expand Down Expand Up @@ -513,7 +514,8 @@ Examples:
(t
(with-protect ("Error: ~a" :condition)
(start-browser urls))))
(unless *run-from-repl-p* (uiop:quit))))
(unless *run-from-repl-p* (uiop:quit)))
*browser*)

(defun load-or-eval (&key remote)
(when remote
Expand Down
35 changes: 35 additions & 0 deletions tests/offline/start.lisp
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
;;;; SPDX-FileCopyrightText: Atlas Engineer LLC
;;;; SPDX-License-Identifier: BSD-3-Clause

(in-package :nyxt/tests)

(define-class dummy-renderer (renderer)
((name "Dummy renderer"))
(:export-class-name-p t)
(:export-accessor-names-p t)
(:accessor-name-transformer (class*:make-name-transformer name)))

(defmethod install ((_ dummy-renderer)) t)

(defmethod uninstall ((_ dummy-renderer)) t)

(setf nyxt::*renderer* (make-instance 'dummy-renderer))

(define-test null-quit ()
(assert-false (nyxt:quit)))

;; See https://github.com/atlas-engineer/nyxt/issues/2754
;; (define-test start-quit-cycle ()
;; (assert-false (progn (nyxt:start :failsafe t) (nyxt:quit))))

;; Fails.
Copy link
Member Author

Choose a reason for hiding this comment

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

@Ambrevar @aartaka @jmercouris

How critical do you think that this is?

For the user that starts Nyxt via the OS shell, it doesn't matter much.

For the developer that starts Nyxt from the REPL it's critical.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, fell under my radar.

It's not critical production-wise, as you pointed out.
Still, this highlights that our startup sequence is not as functional as we would hope, which indicates we wrote poor code :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed!

I'll soon come back to this PR by adding all tests that would capture the invariants of a good startup sequence. If some are not met, then I'll leave them commented out.

Copy link
Member

Choose a reason for hiding this comment

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

Pierre covered it!

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean @jmercouris?

Copy link
Member

Choose a reason for hiding this comment

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

I mean I agree with Pierre's conclusions

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it!

(define-test stateless-headless-argument ()
(nyxt:start :headless t :failsafe t)
(ffi-within-renderer-thread nyxt:*browser*
(lambda () (assert-true nyxt::*headless-p*)))
(nyxt:quit)

(nyxt:start :failsafe t)
(ffi-within-renderer-thread nyxt:*browser*
(lambda () (assert-false nyxt::*headless-p*)))
(nyxt:quit))
Loading