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.

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

Jump to bottom

lua: test the interpreter #177556

Closed
wants to merge 395 commits into from
Closed

lua: test the interpreter #177556

wants to merge 395 commits into from

Conversation

teto
Copy link
Member

@teto teto commented Jun 13, 2022 •
edited
Loading

add some basic tests to the lua interpreter. Some aspects of the nix lua ecosystem are certainly broken so I would like to have some tests to improve it and prevent regressions.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@teto teto marked this pull request as draft June 13, 2022 18:34
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/using-lua-modules/19769/2

@teto teto mentioned this pull request Jun 18, 2022
Open
1 task
@ofborg ofborg bot requested a review from vrthra August 29, 2022 21:35
@teto teto removed the request for review from vrthra September 3, 2022 11:46
@teto teto mentioned this pull request Sep 4, 2022
Closed
@teto teto mentioned this pull request Oct 4, 2022
Closed
31 tasks
roberth
roberth reviewed Oct 4, 2022
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I appreciate your intent to allow your code to be reused, but this is a separate task that will both delay your PR and make your reusable test framework idea less successful because it deserves attention from a different audience of reviewers and collaborators, who may have little interest in lua.

lib/tests/assert.sh Outdated
@@ -0,0 +1,16 @@
# Always failing assertion with a message.
Copy link
Member

Choose a reason for hiding this comment

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

lib/tests is for code that supports the testing of lib itself. Please move this file near where it's used.

pkgs/test/make-wrapper/lib.nix Outdated
@@ -0,0 +1,15 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Please keep it near your test for this PR. You can follow it up with a proposal to make it available to other code later, which is a different goal from testing lua, and needs different reviewers. By splitting the work, your PR will be easier to merge.

pkgs/development/interpreters/lua-5/tests.nix Outdated
Comment on lines 13 to 18
nmt = fetchFromGitLab {
owner = "rycee";
repo = "nmt";
rev = "d2cc8c1042b1c2511f68f40e2790a8c0e29eeb42";
sha256 = "1ykcvyx82nhdq167kbnpgwkgjib8ii7c92y3427v986n2s5lsskc";
};
Copy link
Member

Choose a reason for hiding this comment

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

nmt here is unused code.

@7c6f434c
Merge pull request NixOS#193636 from crabtw/staging
d89b200 
llvmPackages_{14, git}.clang: add nostdlibinc flag
@7c6f434c
Merge pull request NixOS#186575 from stephank/fix/compiler-rt-builtins
711191c 
compiler-rt: build builtins on darwin
@teto teto changed the base branch from master to staging October 13, 2022 21:03
@teto
lua: test the interpreter
6b4fa84 
Test with:
nix-build -A lua.tests pass

Tests are very limited for now, goal is mostly to put the infra into
place and enrich the tests when dealing with lua issues.
add luaPath/luaCpath as passthrough
makes it easier to generate LUA_PATH/LUA_CPATH
@teto teto marked this pull request as ready for review October 13, 2022 21:07
@teto teto requested a review from figsoda October 13, 2022 21:11
@figsoda
Copy link
Member

figsoda commented Oct 13, 2022

does it rely on the changes in staging? if not, perhaps we should rebase it to master

@teto teto marked this pull request as draft October 13, 2022 21:50
@teto teto changed the base branch from staging to master October 13, 2022 21:50
@teto teto closed this Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth left review comments

@figsoda figsoda Awaiting requested review from figsoda

Assignees
No one assigned
Labels
6.topic: golang 6.topic: haskell 6.topic: kernel 6.topic: lua 6.topic: nixos 6.topic: ocaml 6.topic: python 6.topic: qt/kde 6.topic: ruby 6.topic: stdenv Standard environment 6.topic: systemd 6.topic: vim 6.topic: vscode 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.