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

Literate repl tests #10075

Closed
wants to merge 10 commits into from
Closed

Literate repl tests #10075

wants to merge 10 commits into from

Conversation

lf-
Copy link
Member

@lf- lf- commented Feb 25, 2024

Motivation

What if we could write repl tests that were basically just repl sessions? We could test the debugger! We could do a great many things!

This PR brings generic infrastructure for writing literate tests, and a concrete implemention to attach them to the repl. It is unfinished and could use a good refactor pass, but is being presented as-is for comment.

The current state of this is that it works with readline except it does not work under editline, because in the infinite wisdom of the editline developers, no prompts are printed in editline if it's not attached to a tty. I will fix this by either attaching to a pty or implementing a machine readable mode for the repl that always prints some special character as a prompt.

There is an example test using it in the PR.

Context

Do note that someone else wrote something vaguely similar: https://github.com/mobusoperandi/eelco

I didn't use it because I didn't think it was really ideal to depend on someone else's unfinished, out-of-tree test driver for internal tests. Also, as far as I can tell, it looks less likely to be able to support testing the debugger.

The inspiration for this work is: https://bitheap.org/cram/

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added with-tests Issues related to testing. PRs with tests have some priority repl The Read Eval Print Loop, "nix repl" command and debugger labels Feb 25, 2024
@thufschmitt
Copy link
Member

Discussed during the Nix maintainers meeting on 2024-02-26.
This (both the implementation and the tests using it) would be too complex to maintain.
A better solution would be to make the repl code abstract over the IO part, so that we can write the interaction tests directly in C++

  • Might replace tests/functional/lang.sh with this?

    • @roberth: Might not be feasible to infer changes and apply them automatically. lang.sh makes that automatic.
  • We imagine trying to decouple the repl into a "backend" and a "TTY frontend". In addition for this being good for testing (we can mock the later), this is also good for features too. E.g. if we have a WASM build of Nix someday, the repl frontend should be web-based not PTY based, so we would want that same decoupling. It's best when new complexity is useful both for (eventual) feature work and testing, rather than just testing.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-26-nix-team-meeting-minutes-128/40496/1

@fricklerhandwerk
Copy link
Contributor

An alternative to saying "contribution X is too complex to maintain for current maintainers and therefore we won't have it" would be giving contributors of heavy features merge access to maintain it themselves, nailing down code ownership and requiring merges to be approved by code owners to protect against uncoordinated changes. Then this sort of situation ("it's not great yet but we want it, so let's merge and give the author space to develop it") would be tractable.

@lf-
Copy link
Member Author

lf- commented Feb 29, 2024

Discussed during the Nix maintainers meeting on 2024-02-26. This (both the implementation and the tests using it) would be too complex to maintain. A better solution would be to make the repl code abstract over the IO part, so that we can write the interaction tests directly in C++

Well you are in luck because I already did that to the input. The output handling is where I think we would be kinda screwed cuz I need to be able to deal with arbitrary interactions with the logger to test the debugger properly. This is mostly unavoidable.

But if we do wind up writing a better TTY abstraction or something, we can simply delete my process handling stuff, idk??

I am strongly against writing the tests in C++, because it defeats the entire purpose of making the tests easier to write and maintain. You cannot write a tool that automatically updates the C++ files. This is why they have to be in separate files, and why we need an actual parser to parse them.

@lf-
Copy link
Member Author

lf- commented Feb 29, 2024

re:

Might not be feasible to infer changes and apply them automatically. lang.sh makes that automatic.

This is relatively trivial to do, you write a loop that looks for Command syntax tree nodes, and then aligns them with the actual ones in the output, then, if the Output between the one node and the next one is mismatched between expectation and reality, you delete all Output nodes in the expectation there, and insert the ones in the output in their place.

This is the point of the way that I designed this system, I 100% want and plan to do this.

@lf-
Copy link
Member Author

lf- commented Feb 29, 2024

Also incidentally, re implementation complexity, I believe we actually have some quite scuffed regexes to delete terminal codes, which could be deleted and replaced with the DFA I wrote here, so it would be removing some bad code from Nix too :)

@lf- lf- force-pushed the jade/literate-repl-tests branch 3 times, most recently from 3e0a5bb to 7e5558c Compare March 6, 2024 01:16
@lf- lf- mentioned this pull request Mar 6, 2024
@lf- lf- force-pushed the jade/literate-repl-tests branch 2 times, most recently from c309e02 to 5ceadd4 Compare March 10, 2024 08:31
lf- added 9 commits March 12, 2024 18:25
This is in direct preparation for an automation mode of nix repl.
This parser can be reused for other purposes. It's inspired by
https://bitheap.org/cram/

Although eelco's impostor exists https://github.com/mobusoperandi/eelco,
it is not very nice to depend on out of tree testing frameworks with no
way to customize them.
This allows for automating using the repl without needing a PTY, with
very easy to write test files.
This is because they are unrepresentable in the source files with
commentary but not in the output, so we should just eat them in
normalization. It's ok.
@lf- lf- force-pushed the jade/literate-repl-tests branch from 5ceadd4 to 62eecdb Compare March 13, 2024 02:19
@lf- lf- changed the title WIP: literate repl tests Literate repl tests Mar 13, 2024
@lf- lf- marked this pull request as ready for review March 13, 2024 02:49
@infinisil
Copy link
Member

Having looked through the diff, I think this is exactly the sort of work Nix needs right now! Yes it adds some complexity (though it's not that hard to understand and it seems fairly separated from the rest of the codebase), but it doesn't introduce any features and just improves the test suite. Worst case, if it's ever deemed too complex or high-maintenance, it can just be removed again without any direct problems. I'm fully in favor of merging this!

The only actionable feedback I have is that it would be great to have some Markdown files around the source to give an overview of how it works.

@lf-
Copy link
Member Author

lf- commented Mar 14, 2024

i need to rebase this again as I've found bugs in the build system integration. marking as draft till i have more energy. the prereq pr to this is like, objectively good though, please go merge it, regardless of the state of this one.

@lf- lf- marked this pull request as draft March 14, 2024 06:48
@lf-
Copy link
Member Author

lf- commented Mar 26, 2024

I don't really have energy for pushing on this further because the build system integration stuff is such a confusing mess due to the makefile based build system. sorry.

@lf- lf- closed this Mar 26, 2024
@infinisil
Copy link
Member

I guess that's a good argument for NixOS/rfcs#132

@roberth
Copy link
Member

roberth commented Mar 28, 2024

@lf- I think this would be a really good PR!

Maybe we can help you with the build system issue?
Is it in tests/libutil-support or tests/functional/repl_characterization/local.mk?

If it's the prior, maybe you could move that test to libexpr?
Otherwise, I think it'd be ok to use the existing functional test infrastructure for a start, and write a bit of boilerplate to invoke the literate repl tests.

@lf-
Copy link
Member Author

lf- commented Mar 28, 2024

I have some code actually, it's just kind of a nightmare to clean up and I don't have the energy to do it.

@lf-
Copy link
Member Author

lf- commented Mar 28, 2024

The essence of the build system issue is that this needs to look like the plugin test to be able to integrate with the functional tests system. using _RUN is broken for reasons I mostly forget, I think it was related to environment variable setup. I can in principle shuffle the code as necessary, I just don't have the time.

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 2, 2024

I might be able to take on the build system parts

I have some code actually, it's just kind of a nightmare to clean up and I don't have the energy to do it.

feel free to just push what you got, in whatever state it's in.

@lf-
Copy link
Member Author

lf- commented May 6, 2024

apologies for the delay. the specific make stuff was that i needed to add a wrapper script in the functional directory because adding it to _RUN was broken in ways i forgot by now. also it's been entirely rewritten parser wise by now into a recursive descent parser.

we have meson files also, if that's useful. we intend to delete the make files very soon so if those vanish in the next few days, that would be why.

anyway here's the code, i got burned out on backporting it but it should be less hard to copy paste instead of messing about with git:

https://git.lix.systems/lix-project/lix/src/branch/main/tests/functional/repl_characterization

i don't have energy to push this PR forward on this end. very sorry about that.

@lf- lf- deleted the jade/literate-repl-tests branch May 31, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl The Read Eval Print Loop, "nix repl" command and debugger with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants