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

Fix out-of-source testing #14

Merged
merged 1 commit into from
Jun 3, 2021
Merged

Fix out-of-source testing #14

merged 1 commit into from
Jun 3, 2021

Conversation

rosik
Copy link
Contributor

@rosik rosik commented May 30, 2021

The testing setup was overcomplicated. And out-of-source scenario didn't work.

This is another alternative to #12.

By the way, why did you choose tap and not luatest?

@rosik rosik requested a review from Totktonada May 30, 2021 21:53
@Totktonada
Copy link
Member

Totktonada commented Jun 2, 2021

In short: your way is better (at least in context of testing of this module). I would push PR #12 first (it'll highlight weak spots of my approach and make more obvious why your one is better) and then push PR #14 (this one) as refactoring. Okay?

Details below are for historical purposes: they describe points for and against each approach.


Now I obligated to compare two ways to fix the problem and make a balanced decision. (I cannot just choose what I like, it would be a kind of job security action.)

I think it'll be easier to compare resulting code: <current master + PR #12> vs <current master + PR #14 (this one)>, rather than patches.

PR #12

The testing script has a kind of 'relative import' functionality (the weird code hunk based on debug.getinfo() and fio.abspath()). It is like package.setsearchroot('..') (see PR #4193), but works on 1.10 as well. The main reason to bear with this ugly code is to ensure that the code from the repository is tested, not the same module installed into the system: even when I run the test directly (without make test) and not from the repository root. It is maily for local development. I was hit by a problem of this kind once. It works only for in-source build, though (but I use it for local development, so it works for me). Now I mostly work from the repository root as cwd, so I'm ready to get rid of this. (May someone else stuck in such way? Unlikely? Or, if .rocks is present somewhere above?..)

The separate hack with LUA_CPATH is necessary for the out-of-source build, so everything above look as quite specific solution. It seems, I should eleminate the 'relative import' hack, it confuses more than helps.

The runtest.sh script aims to provide autocleaned gitignored working directory for tests. I guess there is a tricky way to implement it in pure CMake, without the wrapper, but, to be honest, I failed on this. And, I guess, the wrapper will look simpler at the end. The reason here is simple: place all those snapshots, xlogs (and temporary files if we'll have some) into particular directory (test/var), not the repository root (which is the case for in-source build). So, it will not mess a developer when not needed, but there is ability to look into the working files.

PR #14 (this one)

No 'relative import' hack. So, no protection from loading an external (system or .rocks) module if you run the test manually (without make test) and not from the binary directory. However we can check, whether tuple/keydef.${SOEXT} (NB: don't lean on ${PWD} environment variable) exists and fail if it doesn't. It is quite optional, though.

No separate gitignored directory for temporary files from testing to investigate after fail. In fact, we don't need it for such simple test. There is nothing interesting in the initial snapshot and in xlogs. And we don't save a log file anyway.

No LUA_CPATH hack, just because we don't need it here. That's good.

memtx_dir = <tempdir> + fio.rmtree(<tempdir>) + wal_mode = 'none' hack to get rid of snaps and xlogs. To be honest, wal_mode = 'none' disquiets me a bit: only part of tarantool functionality is enabled in this mode, so the testing is less powerful. However we don't create any space in the test: we need an initialized box only to load and test collations. So, in this context, it is okay. Maybe even okay for most modules.

The only complain is that there is no any comment for the hack. Even for me, it was not obvious on the first glance, how it works.

NB: Don't forget to remove test/var from .gitignore.


Both ways are hacky in some aspects. But your one is much simpler. I would prefer PR #14 (this one) as the final solution.

However I would prefer to push PR #12 first (as fix) and push PR #14 (this one) afterward (as refactoring). This way it'll be more obvious from the git history, what exactly is wrong with my way. Otherwise it'll be hidden in some closed PR.

Ideally a testing system should solve such problems (or the testing system + its integration with CMake). I'll try to port the test to luatest at some free time, it is interesting. (I guess I'll move test cases that require an uninitialized box to its own file.)


By the way, why did you choose tap and not luatest?

It was quite arbitrary decision. I just chose what had in my hands at the moment of writting. I was in a hurry and had no time for experimenting. The test is mostly copy-paste of the original test from tarantool sources (and the original one is written on tap).

We don't need advanced features from a testing system here. OTOH, in the next my module, where I had a need to spawn processes, I tried luatest and my experience was pretty good.

luatest is going to be the gold standard for tarantool modules and my opinion that this direction is good. However I still want to get more experience around luatest to recommend it as default option (kinda 'choose it if your needs are not VERY specific'). I filed tarantool/test-run#304, in particular, to experiment more around it and gain an experience driven opinion here.


Are you still here? I appreciate it. Sorry for bothering you with all those tiny details. I hate situations, when some decision was made a long time ago by someone, who doesn't work with you anymore (or worked before you), and there are no traces in the noosphere, why the decision was X and not Y. Now a future reader will have a chance to understand what is going on. Sorry again.

It keeps all necessary functionality working:

* The module from the repository (strictly speaking, from the build
  directory) is preferred over one installed into a system or to .rocks.
* The test does not spoil the working directory with *.snap and *.xlog
  files.
* `make test` works as for in-source as well as for out-of-source build.

See details in PR #14.

@Totktonada: Added comments, rewrote the commit message, adjusted
.gitignore. The idea and actual changes are from @rosik.
@Totktonada
Copy link
Member

Rebased after PR #12, PR #11 and PR #13.

However I would prefer to push PR #12 first (as fix) and push PR #14 (this one) afterward (as refactoring). This way it'll be more obvious from the git history, what exactly is wrong with my way. Otherwise it'll be hidden in some closed PR.

I rewrote the commit message as for a refactoring change.

The only complain is that there is no any comment for the hack. Even for me, it was not obvious on the first glance, how it works.

Commented the new trick.

NB: Don't forget to remove test/var from .gitignore.

Removed.

@Totktonada Totktonada merged commit 7dc758d into master Jun 3, 2021
@Totktonada Totktonada deleted the fix-oob branch June 3, 2021 01:20
This was referenced Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants