Test start #2718
Test start #2718
Conversation
tests/offline/start.lisp
Outdated
| (define-test stateless-headless-argument () | ||
| (nyxt:start :headless t :failsafe t) | ||
| ;; TODO find a way to avoid these calls to sleep. | ||
| (sleep 4) |
There was a problem hiding this comment.
Currently the only proper way is via the after-startup-hook.
But clearly it's not convenient for testing. A few options:
- Add a
callbackoption tonyxt:startwhich is executed whenafter-startup-hookis run. This callback could either run the test code or fulfill a promise which the test code is waiting on. - Add an (unexported?) promise slot to browser, and have it fulfilled when
after-startup-hookis run.
This way the caller canforcethe promise (which means "wait for the browser to be started"). - Make
nyxt:startsynchronous, which means we would put all the promise/force logic insidenyxt:startitself and only return from the function once the renderer is started.
The drawback is thatnyxt:startwould now be blocking. But... considering there is nothing to be done in the meantime, is this really a drawback?
There was a problem hiding this comment.
- Finally move
wait-on(-handler)tonhooks? not exactly a solution, but makes tests much prettier.
There was a problem hiding this comment.
I'm not fully satisfied with the wait-on approach: it's still a cumbersome approach that forces nesting over nesting...
The CPS approach is more interesting in this regard: #2054 (comment)
There was a problem hiding this comment.
Or promises/futures and lparallel:plet?
There was a problem hiding this comment.
I think the best and cleaner solution would be to make nyxt:start synchronous.
The drawback is that nyxt:start would now be blocking. But... considering there is nothing to be done in the meantime, is this really a drawback?
No. That's why I think this is the best solution.
This change would also gracefully handle (progn (nyxt:start) (nyxt:quit)).
There was a problem hiding this comment.
@Ambrevar are you interested in working on this or should I do it?
There was a problem hiding this comment.
If you think you have a clear idea of how to implement it, please go ahead :)
There was a problem hiding this comment.
I think the best and cleaner solution would be to make
nyxt:startsynchronous.The drawback is that nyxt:start would now be blocking. But... considering there is nothing to be done in the meantime, is this really a drawback?
No. That's why I think this is the best solution.
This change would also gracefully handle
(progn (nyxt:start) (nyxt:quit)).
Okay, this one feels quite contradictory.
- I understood that you want to make
nyxt:startto happen synchronously. The only clear option I see for that isbt:join-threadon the thread thatnyxt:startspawns.- An alternative solution would be waiting on some
after-startup-hook(or some alternative) innyxt:start, but that is architecturally impossible, because*browser*is not yet initialized when we runnyxt:start.
- An alternative solution would be waiting on some
- Given that we join threads, one cannot simply do
(progn (nyxt:start) (nyxt:quit)), becausenyxt:startdoes not terminate untilnyxt:quitis called. Which is a vicious circle.- If we wait on some event (which is quite bad for an startup routine), then we'd be basically baking the waiting logic from the tests into the
nyxt:start. And it does not belong there.
- If we wait on some event (which is quite bad for an startup routine), then we'd be basically baking the waiting logic from the tests into the
- All in all, I don't see much use in making
nyxt:startsynchronous, because it's either- Creating inconveniences for the otherwise trivial use-case (
bt:join-threadI can do myslef), or - Introducing backwards references to the extremely fragile startup sequence.
- Creating inconveniences for the otherwise trivial use-case (
I must be missing something here. I mean, I'd like to make our startup more reproducible, but not in those two ways I'm imagining, and not necessarily by making nyxt:start synchronous/blocking, so there should be a better way you're conjuring, @aadcg.
There was a problem hiding this comment.
Honestly, I don't want to work on the architecture of start.lisp. There is too much historical background that I lack, while @aartaka and @Ambrevar know it quite well.
@Ambrevar suggestion on making nyxt:start synchronous seems sane to me, because I don't see the point in having the ability to do something while the renderer is loading (and @Ambrevar seems to agree).
With regards to the semantics of (progn (nyxt:start) (nyxt:quit) I think it should mean: "Start Nyxt, block it while the renderer loads and then, when nyxt:start exists, proceed and run nyxt:quit". I'm sure this is possible to do. But I understand that it would require re-thinking a lot of the architecture.
I can help with testing and making sense of the start process of course.
To answer @Ambrevar's comment:
If you think you have a clear idea of how to implement it, please go ahead :)
No, I have no clear idea. I just have a clear idea of how things ought to be :)
|
GitHub did this for me, so yes, this should remain open. |
|
Yes, there is lots of work to be done on this front. Not on testing itself, but on making a proper start implementation. And the implementation specification is dictated by the test I write tests (that ought to pass). |
|
Makes sense if |
Currently it returns NIL, but I agree that it should return |
| ;; (define-test start-quit-cycle () | ||
| ;; (assert-false (progn (nyxt:start :failsafe t) (nyxt:quit)))) | ||
|
|
||
| ;; Fails. |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What do you mean @jmercouris?
There was a problem hiding this comment.
I mean I agree with Pierre's conclusions
|
Please reopen when ready. |
Fixes #2710
Discussion
Work in progress. I'd like you to take a look at the strategy I'm taking. @Ambrevar @aartaka @jmercouris
Checklist:
Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.
cd /path/to/nyxt/checkout git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser:documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)changelog.lispwith my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).migration.lispentry for all compatibility-breaking changes.