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

[RFC] Update busted to version 2. #1098

Merged
merged 1 commit into from Sep 12, 2014

Conversation

Projects
None yet
6 participants
@fwalch
Copy link
Member

fwalch commented Aug 21, 2014

Busted 1.x doesn't seem to be supported anymore (no 1.x branch on Github), so I think we have to update sooner or later. I am not sure why the changes to the test files are necessary (or rather, why it worked before), maybe someone can shed light on that?

Also (and this was why I originally tried this) this fixes #1031 (and obsoletes #1092). But (and I only thought about this after I had already created these commits) the color_terminal output from #983 won't work with this anymore.

From https://github.com/Olivine-Labs/busted/blob/master/busted/outputHandlers/utfTerminal.lua#L48-L50: a traceback is printed in the default utfTerminal output handler if verbose is set, so I changed this to be the default (for local builds as well):

Failure → /home/florian/Projects/neovim/test/unit/fileio_spec.lua @ 19
file_pat functions file_pat_to_reg_pat returns ^path$ regex for literal path input
/home/florian/Projects/neovim/test/unit/fileio_spec.lua:20: Expected objects to not be the same. Passed in:
(string) '^path$'
Did not expect:
(string) '^path$'

stack traceback:
    /home/florian/Projects/neovim/test/unit/fileio_spec.lua:19: in function </home/florian/Projects/neovim/test/unit/fileio_spec.lua:12>

If there are no errors, the output is the same as non-verbose. Output handlers other than utfTerminal and plainTerminal don't use the verbose flag.

Would this be an acceptable replacement for color_terminal for now? An improved output handler could be sent as a PR to busted, maybe it would even get into 2.0.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 21, 2014

I think it would be better to squash at least the first two commits together. In that way, we always have a system that builds and tests on each commit. This could be handy for git bisect, for example.

Otherwise, I think this needs to happen at some point, so goob job :). Perhaps @ZyX-I has some input since he's the one who made the alternate output for Busted v1 (and mentioned that the plugin system had changed).

@fwalch fwalch force-pushed the fwalch:update-busted branch from b1949f4 to 3cd627f Aug 21, 2014

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 21, 2014

I think it would be better to squash at least the first two commits together.

👍

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 21, 2014

Removed exports following @aktau's suggestion.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 21, 2014

👍 💯 but a unit test failed. Is it passing on your local machine?

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 22, 2014

@ZyX-I Do you have any reservations about this?

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 22, 2014

Can this failure be caused by old version of busted in deps repo? https://github.com/neovim/deps/tree/master/share/lua/5.1/busted/output has utf_terminal.lua but https://github.com/Olivine-Labs/busted/tree/master/busted/outputHandlers has utfTerminal.lua and error message is:
/opt/neovim-deps/share/lua/5.1/busted/core.lua:67: /opt/neovim-deps/share/lua/5.1/busted/core.lua:95: module 'busted.output.utfTerminal' not found

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 22, 2014

@Hinidu That should be it it.. also tested it locally (on master, using BUSTED_PRG busted_bootstrap and output handler utfTerminal), and it seems to fall back to the default output handler if an invalid one is specified.

In the PR branch, the tests pass locally. They should work on Travis after neovim/deps has been updated.

Since we're now always using the default output handler, the explicit BUSTED_OUTPUT_TYPE could also be removed to make it work with old and new busted. Then it'd have to be re-added if we create a custom output handler, though.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 22, 2014

I will update deps repos tonight so this can hopefully be merged.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 23, 2014

Result looks OK except for this part:

file_pat functions file_pat_to_reg_pat returns ^path$ regex for literal path input
                  ^                   ^

: here it used to be / and not just space.

But you say you want to update for 2.0.rc2. I would rather wait for release.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Aug 23, 2014

Given that space actually makes sense file_pat functions header should be renamed to file_pat function.

@fwalch fwalch force-pushed the fwalch:update-busted branch from 2d600d7 to c3a0f6f Aug 29, 2014

@justinmk justinmk referenced this pull request Aug 29, 2014

Merged

[RFC] update deps #3

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 29, 2014

@fwalch I restarted the build for this PR after updating the deps repo. If it succeeds can you rebase?

edit: Nevermind, I just noticed that you already rebased. Thanks :)

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 29, 2014

The 32-bit and unittest builds are failing on busted_bootstrap, which makes sense because we haven't merged this PR yet.

However, the clang and python builds are complaining about this:

-- Checking Lua interpreter /opt/neovim-deps/bin/luajit
-- [/opt/neovim-deps/bin/luajit] The 'lpeg' lua package is required for building Neovim
CMake Error at CMakeLists.txt:117 (message):
  A suitable Lua interpreter was not found

Probably something I messed up in the deps, don't know what yet though.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 29, 2014

Probably something I messed up in the deps, don't know what yet though.

If I were to guess, I'd say that deps were rebuild, but not everything was staged into version control. I think using git add -f . might fix the problem

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 29, 2014

I deleted my .gitexcludes beforehand, but I'll try again.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 29, 2014

Sigh, I just realized this is a 32-bit VM. Can someone else rebuild the deps on a 64-bit VM? I don't have one on this machine right now.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 29, 2014

Sigh, I just realized this is a 32-bit VM. Can someone else rebuild the deps on a 64-bit VM? I don't have one on this machine right now.

In 30 minutes I will be at home then I will do it

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 29, 2014

@tarruda / @justinmk: rc3 is out, so I changed the PR to use that. Can you verify that tests run locally (make unittest) before pulling? I don't have access to my laptop right now, but on my current machine tests somehow fail for both rc2 (which worked before on my laptop) and rc3.

Edit: Well, failures will of course also be reported by Travis when the deps are updated.

@fwalch fwalch changed the title [RFC] Update busted to 2.0.rc2. [RFC] Update busted to version 2. Aug 29, 2014

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 29, 2014

Edit: Well, failures will of course also be reported by Travis when the deps are updated

I will update the deps repository with rc3 and and then merge this PR

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 30, 2014

@fwalch the unit tests are failing with this new busted version, here's the output:

188 successes / 9 failures / 0 errors / 0 pending : 0.0 seconds

Failure → /opt/neovim-deps/neovim/test/unit/path_spec.moon @ 138
path function invocation_path_tail returns the executable name of an invocation given a relative invocation
Nil error
stack traceback:
        /opt/neovim-deps/neovim/test/unit/path_spec.moon:138: in function </opt/neovim-deps/neovim/test/unit/path_spec.moon:123>

Failure → /opt/neovim-deps/neovim/test/unit/garray_spec.moon @ 297
garray ga_concat_strings_sep returns an empty string when concatenating an empty array
Nil error
stack traceback:
        /opt/neovim-deps/neovim/test/unit/garray_spec.moon:297: in function </opt/neovim-deps/neovim/test/unit/garray_spec.moon:296>

Failure → [C] @ -1
garray ga_concat_strings_sep can concatenate a non-empty array
Nil error
stack traceback:

Failure → /opt/neovim-deps/neovim/test/unit/path_spec.moon @ 154
path function invocation_path_tail does not count arguments to the executable as part of its path
Nil error
stack traceback:
        /opt/neovim-deps/neovim/test/unit/path_spec.moon:154: in function </opt/neovim-deps/neovim/test/unit/path_spec.moon:123>

Failure → /opt/neovim-deps/neovim/test/unit/garray_spec.moon @ 285
garray ga_concat_strings returns an empty string when concatenating an empty array
Nil error
stack traceback:
        /opt/neovim-deps/neovim/test/unit/garray_spec.moon:285: in function </opt/neovim-deps/neovim/test/unit/garray_spec.moon:284>

Failure → [C] @ -1
garray ga_concat_strings can concatenate a non-empty array
Nil error
stack traceback:

Failure → [C] @ -1
garray ga_remove_duplicate_strings sorts and removes duplicate strings
Nil error
stack traceback:

Failure → [C] @ -1
garray ga_append can append strings to a growing array of strings
Nil error
stack traceback:

Failure → /opt/neovim-deps/neovim/test/unit/path_spec.moon @ 143
path function invocation_path_tail returns the executable name of an invocation given an absolute invocation
Nil error
stack traceback:
        /opt/neovim-deps/neovim/test/unit/path_spec.moon:143: in function </opt/neovim-deps/neovim/test/unit/path_spec.moon:123>
CMake Error at /opt/neovim-deps/neovim/cmake/RunUnittests.cmake:13 (message):
  Unit tests failed.

For now I will simply push a fix with the missing 64-bit dependencies, perhaps you'd like to send a PR with the updated deps yourself? Here are some steps you may follow(in case you don't know how):

  • Clone neovim/deps to /opt/neovim-deps(change ownership of /opt/neovim-deps to avoid needing root permissions)
  • Create a new branch for the PR
  • run ./build.sh fwalch/neovim.git update-busted to rebuild the dependencies using this PR

After it's built, you can rebuild nvim using /opt/neovim-deps as a source for dependencies. To do that, it might be useful to define the following shell function in your .{bash,zsh}rc:

set_nvim_env() {
    local prefix="$1"
    eval $($prefix/bin/luarocks path)
    export PATH="$prefix/bin:$PATH"
    export PKG_CONFIG_PATH="$prefix/lib/pkgconfig"
    export USE_BUNDLED_DEPS=OFF
}

Then from your local neovim repository:

rm -rf build
set_nvim_env /opt/neovim-deps
make unittest

You can also create a temporary commit in this PR to change the dependencies repository to your own version(so you can test with travis)

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 30, 2014

Fixed 64-bit dependencies: neovim/deps@b18127a

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 30, 2014

It seems to be related to Moonscript handling & FFI (e.g. ffi.C.geteuid is nil in some test, which explains one of the Nil errors above) in busted.. the following works:

make distclean
make deps
# convert everything to lua
find test/unit -name "*.moon" -exec ./.deps/usr/bin/moonc {} \+
find test/unit -name "*.moon" -exec rm {} \+
# run tests
make unittest

197 successes / 0 failures / 0 errors / 0 pending : 0.0 seconds

Unfortunately, the converted test files are not very readable.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 30, 2014

Unfortunately, the converted test files are not very readable.

Ok I will convert all remaining moonscript tests to lua and manually format everything. We simply can't continue using moonscript if it's causing tests failures.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 30, 2014

Ok I will convert all remaining moonscript tests to lua and manually format everything. We simply can't continue using moonscript if it's causing tests failures.

Don't forget to grep for moon(script) because there might be some spurious references:

formatc.lua:218
third-party/CMakeLists.txt:153-157, 164, ...

't will speed up the build-from-scratch by a tiny margin as well :).

@fwalch fwalch force-pushed the fwalch:update-busted branch from c3a0f6f to 9087b0d Aug 30, 2014

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 30, 2014

Ok I will convert all remaining moonscript tests to lua and manually format everything. We simply can't continue using moonscript if it's causing tests failures.

Updated the PR to not touch the moonscript files to avoid merge conflicts.

@tarruda tarruda referenced this pull request Aug 31, 2014

Closed

[RDY] Drop moonscript #1128

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 31, 2014

Updated the PR to not touch the moonscript files to avoid merge conflicts.

Just merged #1128 @fwalch so feel free to rebase :).

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 31, 2014

Just merged #1128 @fwalch so feel free to rebase :).

Don't have a PC til Friday, and cloning onto my mobile failed at first attempt, so I'm not sure if I can do it.. I'll try again tomorrow :-D

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Sep 1, 2014

Don't have a PC til Friday, and cloning onto my mobile failed at first attempt, so I'm not sure if I can do it.. I'll try again tomorrow :-D

Don't worry, let's see on friday ;).

@fwalch fwalch force-pushed the fwalch:update-busted branch from 9087b0d to 604ad81 Sep 5, 2014

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Sep 5, 2014

Rebased.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Sep 5, 2014

Unable to open output module; requested option '--output=utfTerminal'.�[0m
/opt/neovim-deps/share/lua/5.1/busted/core.lua:67: /opt/neovim-deps/share/lua/5.1/busted/core.lua:95: module 'busted.output.utfTerminal' not found:No LuaRocks module found for busted.output.utfTerminal
no field

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Sep 5, 2014

That's because the deps are still busted 1.10.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Sep 6, 2014

I will try updating neovim-deps with the new busted now

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Sep 6, 2014

@fwalch I'm still getting unit tests errors in this branch, I wonder what could be causing them. You can try them by adding a temporary commit that uses the busted2 neovim-deps branch:

diff --git a/.ci/common.sh b/.ci/common.sh
index a31b306..e6663aa 100644
--- a/.ci/common.sh
+++ b/.ci/common.sh
@@ -60,7 +60,7 @@ install_prebuilt_deps() {
        # install prebuilt dependencies
        if [ ! -d /opt/neovim-deps ]; then
                cd /opt
-               sudo git clone --depth=1 git://github.com/neovim/deps neovim-deps
+               sudo git clone --depth=1 -b busted2 git://github.com/tarruda/neovim-deps neovim-deps
                cd -
        fi
 }

It's a mistery what is causing these errors but here's some copy/paste to test locally:

git clone -b update-busted git://github.com/fwalch/neovim.git fwalch-neovim
cd fwalch-neovim
make unittest
@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Sep 6, 2014

Looking into this now. Seems to be isolated to garray_spec.lua.

on OS X:

$ ./.deps/usr/bin/busted -v -o utfTerminal --lpath=/Users/justin/dev/neovim/build/?.lua test/unit/
●●●●●●●●●E94: No matching buffer for _test_
E94: No matching buffer for _test_
E94: No matching buffer for _test_●
E94: No matching buffer for test●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●/Users/justin/dev/neovim/.deps/usr/bin/luajit: ./test/unit/garray_spec.lua:74: attempt to call global 'ga_itemsize' (a nil value)

on 32-bit ubuntu:

$ ./.deps/usr/bin/busted -v -o utfTerminal --lpath=/home/vagrant/neovim/build/?.lua test/unit/
●Segmentation fault (core dumped)
@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Sep 6, 2014

@tarruda I think I'm pretty close to working this out. Mostly just some messed up variable scopes.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Sep 6, 2014

PR here #1144

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Sep 11, 2014

@fwalch I believe gcc-ia32.sh also needs -DBUSTED_OUTPUT_TYPE=color_terminal removed to avoid this failure:

#1144 (comment)

@fwalch fwalch force-pushed the fwalch:update-busted branch from 604ad81 to 03ed2ee Sep 11, 2014

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Sep 11, 2014

@fwalch I believe gcc-ia32.sh also needs -DBUSTED_OUTPUT_TYPE=color_terminal removed to avoid this failure:

You're right, I missed that. Updated the PR and rebased from master.

@fwalch fwalch force-pushed the fwalch:update-busted branch from 03ed2ee to 9f3c5a8 Sep 11, 2014

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Sep 11, 2014

I'll create a PR to neovim/deps with updated dependencies from Travis, then the build should pass.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Sep 11, 2014

Looking forward to it! I'm sure you know, but just a reminder that it needs to be run on a 64-bit system.

Update busted to 2.0.rc3.
Default to verbose output to show more information in case of errors.
Fix #1031.
@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Sep 11, 2014

Created neovim/deps#4, see there for more explanations.

justinmk added a commit that referenced this pull request Sep 12, 2014

Merge pull request #1098 from fwalch/update-busted
Update busted to version 2.

@justinmk justinmk merged commit 042aca6 into neovim:master Sep 12, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

@fwalch fwalch deleted the fwalch:update-busted branch Sep 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment