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

Improve PJDFSTest suite #59

Open
saidsay-so opened this issue Mar 18, 2022 · 23 comments
Open

Improve PJDFSTest suite #59

saidsay-so opened this issue Mar 18, 2022 · 23 comments

Comments

@saidsay-so
Copy link

Hi!
I would be interested in handling the rewriting of the test suite for GSoC.
I saw the experimental start and would like to add some highlights for the questions raised in the PR, and in addition to the GSoC summary:

Is it worthwhile? There are a lot of tests to convert

In addition to make debugging easier, it could also lower the entry barrier for potential contributors/reviewers.
As mentioned in the summary, sh is harder to debug but also to review, with sometimes surprising behavior.
Also, if we rewrite the tests, duplications due to the lack of parameterization and inheritance would probably not make the number of tests to convert a big deal.

Are the C bindings worth the trouble for all functions? I initially
planned to call the function-under-test directly with C, like the
original pjdfstest. But Python's os module is very close to C. For
example, os.symlink is basically the same thing as symlink(2) except
that it accepts Python paths as arguments instead of C strings, and it
raises exceptions on error rather than setting errno.

It depends on the language that will be finally chosen, but for example, Python implements almost all POSIX syscalls with calling conventions very similar to those of C.
From what I have checked, the only missing that are present in pdjfstest.c are these ones:

  • openat
  • unlinkat
  • mkdirat
  • linkat
  • symlinkat
  • renameat
  • mkfifoat
  • mknodat
  • bind
  • bindat
  • connectat
  • fchmodat
  • fchownat
  • fchflags
  • chflagsat
  • fstatat
  • lpathconf
  • utimensat

(The bold functions have tests.)

Though, to be fair, most of those -at functions are replaced by the dir_fd parameter (except for bindat and connectat).
So, the only functions which would potentially need bindings are lpathconf, bindat and connectat.

This would also allow us to improve the documentation, which for the moment (no offense!) is rather scarce.

Language choice

Python

Pros

  • Easier to review
  • Syscalls availability (os module)
  • Larger community

Cons

  • Performance: Python is definitely not the best in terms of performance, but does it really matter for our case, since we mostly test IO?
  • Setup

Rust

Pros

  • Performance
  • Binary: Tests could be compiled into one binary and distributed this way, instead of having to setup an environment
  • Fearless concurrency
  • Safety

Cons

  • Rely on external dependency: not sure of how much this is a con, but to use syscalls, we cannot rely on the stdlib since it provides no guarantees (and wasn't built for this). But we can use for example nix or rustix.
  • Community: Not really a big problem (I'm myself a huge Rust advocate!), but the community is rather small compared to the others

Go

Pros

  • Syscalls availability (syscall module)
  • Performance
  • Binary: Same as for Rust

Cons

  • Not so fearless concurrency
  • Not so safe (debatable?)

ATF integration

I maybe misunderstood something, but I'm not sure of why is it needed to support ATF, given the fact that kyua supports TAP (and even plain) tests and that it seems to produce reports even for non-ATF tests.
A Kyuafile would probably be sufficient to output reports.

@asomers
Copy link
Collaborator

asomers commented Mar 18, 2022

Bonjour! I assume you saw the GSoC proposal at https://wiki.freebsd.org/SummerOfCodeIdeas#PJDFSTest_rewrite ? I agree with what you said regarding debugability, lower barrier to entry, and inheritance. And I agree that Python's os module will suffice for most syscalls. I really don't think Python's performance will be a problem. For one thing, we don't need high performance. And for another, Python is much faster than sh, which we're currently using.
And I too love Rust! I don't think it would be a problem to depend on something like Nix (BTW, I'm a Nix maintainer), not do I think that the community would be too small. However, I do think that the need to compile would slow down development. And I know that Rust lacks a suitable test framework. There's no currently available Rust test framework that provides a way to skip tests at runtime, for example. You would have to write your own. Still, this is going to be your project, so you can decide whether to take on that extra work.
Regarding ATF, it isn't strictly needed. Kyua can call plain tests just fine. However, it won't display detailed results, just a single pass/fail/skip. So deeper ATF integration would be nice to have, but not required.

@saidsay-so
Copy link
Author

Bonjour! I assume you saw the GSoC proposal at https://wiki.freebsd.org/SummerOfCodeIdeas#PJDFSTest_rewrite ?

Yes, it was this one that I saw!

And I too love Rust! I don't think it would be a problem to depend on something like Nix (BTW, I'm a Nix maintainer), not do I think that the community would be too small.

What a coincidence!

However, I do think that the need to compile would slow down development. And I know that Rust lacks a suitable test framework. There's no currently available Rust test framework that provides a way to skip tests at runtime, for example. You would have to write your own. Still, this is going to be your project, so you can decide whether to take on that extra work.

Hmm, you raised important points. I thought that rstest could be a good candidate for the testing framework, but it doesn't support features which would make it suitable for us. The major blocker for writing a suitable framework seems to be that the custom test framework implementation is incomplete (and available only on Nightly). As for the compilation, I agree that the infamous compile times would effectively slow down the development. I will try to write a prototype, but I'm unsure that Rust is a good fit here :(. Python seems to be the best choice then.

Regarding ATF, it isn't strictly needed. Kyua can call plain tests just fine. However, it won't display detailed results, just a single pass/fail/skip. So deeper ATF integration would be nice to have, but not required.

Oh, thanks, I understand now. I will definitely add ATF integration, it could also be beneficial for the upstream project if I abstract it enough. I think of implementing it as a pytest plugin.

@asomers
Copy link
Collaborator

asomers commented Mar 19, 2022

Yeah, if we do this in Rust I think we would be unable to rely on Custom Test Framework. But in fact, what we're aiming for isn't a "test" as far as Cargo is concerned. Instead, we want a binary that can be reused by itself, outside of Cargo. That means we needn't be held up by CTF compatibility. OTOH, it means that we would need to reimplement all of test collection ourselves.

I actually wrote a PoC integration for ATF and Python several years ago. I could dig it up again, but I would advise you not to worry about that part. It is strictly optional.

@saidsay-so
Copy link
Author

So, I did some research on how we could implement test collection ourselves, and cumbersome is a euphemism…
It is impossible to do it just with an attribute-like macro, as I first thought.
We have (ugly?) solutions to circumvent this:

  • Rely on CTF, but as we said before, CTF is not stable yet, so we would have to use the Nightly channel.
  • Use linker sections (or code before main). We could aggregate the tests in a slice. However, it is platform specific (although Linux and FreeBSD are supported).

@asomers
Copy link
Collaborator

asomers commented Mar 23, 2022

Does the standard test framework use one of those techniques? Another option is to abandon test collection, and do what the Criterion benchmark framework does. It requires you to supply each test case's name to a macro that will generate the main function.
https://bheisler.github.io/criterion.rs/book/user_guide/migrating_from_libtest.html

@saidsay-so
Copy link
Author

AFAIK, the tests are collected directly by the compiler, so definitely not one of those techniques.

@saidsay-so
Copy link
Author

saidsay-so commented Mar 23, 2022

Does the standard test framework use one of those techniques? Another option is to abandon test collection, and do what the Criterion benchmark framework does. It requires you to supply each test case's name to a macro that will generate the main function. https://bheisler.github.io/criterion.rs/book/user_guide/migrating_from_libtest.html

We can use this, that's too bad however that we can't directly collect them.

@asomers
Copy link
Collaborator

asomers commented Apr 7, 2022

@musikid as you've probably noticed, the GSoC application period is officially open. If you're still interested, don't forget to submit your application by 19-April!

@saidsay-so
Copy link
Author

@asomers Yes, I'm still interested! I'm currently experimenting different workflows to know which one could be better for the project.
For now, I think of doing the test collection using linker sections. It provides the benefit of having only to declare an attribute, which we could wrap with our own one (using proc_macro_nested).
As for running the tests in parallel, I was evaluating some options:

  • rely on kyua, since it seems to have support for parallel processing. The support seems to be experimental, though…
    It would also remove the self usable binary.
  • use cargo-nextest's core package (unsure of how it can be customized).
  • write our own test runner. This would give us more flexibility, but I'm not sure of how I underestimate the task…

If you could give me some suggestions, I would be glad to hear.
Thank you for your help!

@asomers
Copy link
Collaborator

asomers commented Apr 8, 2022

Thanks for the tip about cargo-nextest. I hadn't heard of it. After musing for a while, I think some combination of options 1 and 3 would probably be the best. We'll have to have our own test runner, but it needn't implement multithreading. But as long as it supports a -l (list test cases) option, and it will execute a test case supplied as the first argument, then Kyua can invoke it. In other words, it's pretty easy to integrate with Kyua. For single-threaded use, you can have it run all test cases when invoked without options. Kyua will never do that.
https://www.freebsd.org/cgi/man.cgi?query=atf-test-program&apropos=0&sektion=1&manpath=FreeBSD+14.0-current&arch=default&format=html

@asomers
Copy link
Collaborator

asomers commented Apr 8, 2022

Also, don't sweat the linker magic. That could open up quite a can of worms. It's easier just to follow Criterion's example and use a macro to declare test cases. The linker stuff would be nice, but it's lower priority than everything else.

@saidsay-so
Copy link
Author

Thanks for the tip about cargo-nextest. I hadn't heard of it. After musing for a while, I think some combination of options 1 and 3 would probably be the best. We'll have to have our own test runner, but it needn't implement multithreading. But as long as it supports a -l (list test cases) option, and it will execute a test case supplied as the first argument, then Kyua can invoke it. In other words, it's pretty easy to integrate with Kyua. For single-threaded use, you can have it run all test cases when invoked without options. Kyua will never do that. https://www.freebsd.org/cgi/man.cgi?query=atf-test-program&apropos=0&sektion=1&manpath=FreeBSD+14.0-current&arch=default&format=html

Yes, I wrote a POC for an ATF wrapper for pytest, and the interface is simple. So, I will not focus on adding multithreading.

Also, don't sweat the linker magic. That could open up quite a can of worms. It's easier just to follow Criterion's example and use a macro to declare test cases. The linker stuff would be nice, but it's lower priority than everything else.

Let the worms' can sealed, got it!

@saidsay-so
Copy link
Author

I published a first draft of the proposal! I should have finished it for the weekend.

@asomers
Copy link
Collaborator

asomers commented Apr 16, 2022

Is it online somewhere were I can see it?

@saidsay-so
Copy link
Author

I published it on the GSoC dashboard. Do you have access to it?

@asomers
Copy link
Collaborator

asomers commented Apr 16, 2022

I think so, but I couldn't find it. Honestly, that site is not very easy to navigate. Could you please send a direct link?

@saidsay-so
Copy link
Author

@asomers
Copy link
Collaborator

asomers commented Apr 16, 2022

Nope. I get an "Access Denied". Maybe mentors aren't allowed to view proposals that are still in the draft stage?

@saidsay-so
Copy link
Author

They should, normally, to suggest modifications (as Google says). I gave you access to the GitHub repo in which I write (I made it private for now, since it's a draft).

@asomers
Copy link
Collaborator

asomers commented Apr 16, 2022

Ok, I figured out how to find it. Your direct link gives me a 403 error, but I found it by browsing through the FreeBSD project's page.

@saidsay-so
Copy link
Author

Oh, perfect!

@asomers
Copy link
Collaborator

asomers commented Apr 19, 2022

I read the proposal that you uploaded today, and I have three comments:

  • proc macros are tricky. And I don't see anything in the description of #[pjdfs_test] that can't be done with regular runtime code. So unless you already have a good idea of how to implement that macro, I suggest you leave it to the end.
  • Do you already have an idea for a how a test can report that it is skipped? You'll need to know before you can write #[pjdfs_test].
  • You should move ATF support to the very end, after writing all test cases. It should be considered a stretch goal.

@saidsay-so
Copy link
Author

saidsay-so commented Apr 19, 2022

I read the proposal that you uploaded today, and I have three comments:

  • proc macros are tricky. And I don't see anything in the description of #[pjdfs_test] that can't be done with regular runtime code. So unless you already have a good idea of how to implement that macro, I suggest you leave it to the end.

  • Do you already have an idea for a how a test can report that it is skipped? You'll need to know before you can write #[pjdfs_test].

Got it, I will avoid the proc macros then.

  • You should move ATF support to the very end, after writing all test cases. It should be considered a stretch goal.

Sorry, I thought that I already moved it to the end!

Thank you for the advices and the time you took to read it!

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

No branches or pull requests

2 participants