search

vn971 / rua

Build tool for Arch Linux providing control, review and jailed build options
GNU General Public License v3.0
424 stars 42 forks source link

Delete leftovers after a successful install #146

Closed refaelsh closed 2 years ago

refaelsh commented 3 years ago

This PR is in reference to #145. Please be gentle :-)

P.S. Made in Vim with Love.

refaelsh commented 3 years ago

I tried using you rm_rf crate. I've change the IFolderDeleter::delete_folder() function return value to use your struct like this:

pub trait IFolderDeleter {
    fn delete_folder(&self, path: &PathBuf) -> rm_rf::Result<()>;
}

And I get the following error from the compiler:

   Compiling rua v0.17.18 (/home/refaelsh/repos/rua)
error[E0603]: type alias `Result` is private
  --> src/folder_deleter.rs:11:52
   |
11 |     fn delete_folder(&self, path: &PathBuf) -> rm_rf::Result<()>;
   |                                                       ^^^^^^ private type alias
   |
note: the type alias `Result` is defined here
  --> /home/refaelsh/.cargo/registry/src/github.com-1ecc6299db9ec823/rm_rf-0.6.0/src/lib.rs:4:5
   |
4  | use crate::error::Result;
   |

Please advice.

vn971 commented 3 years ago

@refaelsh Sorry again for the slow reply, it seems that rm_rf library itself was missing proper "public" imports: https://github.com/vn971/rm_rf/commit/5d6694bf2b93dd629fbfac4399bdea13151b2ac8

If you change the rm_rf version to latest (0.6.1), the code should start compiling.

BTW, can you please explain a bit, what was the idea behind IFolderDeleter, can't we just get away with calling rm_rf directly? Or you wanted to gradually get away from it by first just replacing the implementation?

refaelsh commented 3 years ago

@refaelsh Sorry again for the slow reply,

This is perfectly ok :-) Thank you :-)

If you change the rm_rf version to latest (0.6.1), the code should start compiling.

It compiles now.

can't we just get away with calling rm_rf directly?

We can. But we should not.

BTW, can you please explain a bit, what was the idea behind IFolderDeleter

Its an interface. In Java/C# one would use the interface keyword and in C++ for example one would use an Abstract Class (since C++ lacks the interface construct). This approach has several advantages:

vn971 commented 3 years ago

Ah, if we want to e.g. test it, then it makes sense indeed (e.g. "when you run install ABC with conditions XYZ, a call to remove directory KLM needs to be made"). Good stuff then! I'll read the new MR a bit later

refaelsh commented 3 years ago

Question please. I wanted to see what/where is the CI/CD and I cant find it. The file .travis.yml cant be it because it does not contains cargo build --release. I also saw release.sh, but it also does not contains cargo build --release. Can you please point me in the right direction?

vn971 commented 3 years ago

Hey, took a while, it's always a bit harder to write reviews if they are not approving ones.

The main problem that I see with the PR as it's currently written, is what it aims to do and what it does.

If I understand the intent correctly, we want to introduce tests for package installation and directory deletion. I see that we're creating the traits for directory deletion, which many different structures (or mocks) could then implement. If I understand it correctly, the common approach, however, would be then to also use this abstract trait all the way up in the dependency tree with something like dependency injection.

For example, let's say we want to test how RUA works internally. Our "written in English" test would sound, I assume, smth like: "whenever a package is installed, a call to remove the build directory should be made". In terms of traits and mocks, it should be a "package installer" function that accepts a "directory deleter" trait, and calls it when required. Now we want to test the "package installer" function, and instead of giving the function a real "directory deleter", we would provide a fake one. We would then ask the "package installer" to proceed. The mock would tell us if "directory deleter" was indeed called in the process, and possibly check which arguments were used.

Going further into the dependency tree, the function that uses "package installer" could also take "package installer" as a trait, and in testing we could override that as well. That would be the full DI / testing approach.

Now we're getting to the problem. In this PR, "package installer" does not really accept a "directory deleter" trait. We're abstracting away from "directory deleter" by using a trait, but we can only ever use one specific "directory deleter", which is hardcoded in the current PR in line 60 of actions_install.rs: https://github.com/vn971/rua/pull/146/files#diff-863878877eeb9159c0270d8aa71de01d3281a4b761037e67af3430957626977fR60 We didn't test "package installer"-s behavior, we only tested a self-contained "directory deleter" behavior.

That's the problem that I saw, I'll write a separate response on where the code could go from there

vn971 commented 3 years ago

The solutions on how to address the problem, as far as I see, could be:

Note that if you'd like to proceed with the second approach, some other things would be good to fix:

vn971 commented 3 years ago

WRT testing, I've raised an issue here: https://github.com/vn971/rua/issues/151

vn971 commented 3 years ago

release.sh has cargo publish, which implies cargo build --release. This script, however, is for maintainers only, and normal user wouldn't benefit from that. The actual cargo build --release happens when you install rua as an AUR package here: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=rua#n33

Or when you follow README.md, choose the Rust type of installation and run cargo install rua: https://github.com/vn971/rua#install-the-rust-way

vn971 commented 3 years ago

Hope any of that helps...

refaelsh commented 3 years ago

Hey, took a while, it's always a bit harder to write reviews if they are not approving ones.

Thats ok, I completely understand you :-)

The main problem that I see with the PR as it's currently written, is what it aims to do and what it does.

If I understand the intent correctly, we want to introduce tests for package installation and directory deletion. I see that we're creating the traits for directory deletion, which many different structures (or mocks) could then implement. If I understand it correctly, the common approach, however, would be then to also use this abstract trait all the way up in the dependency tree with something like dependency injection.

That would be te correct way - "all the way up in the dependency tree". I was just afraid to introduce such a big change, its a kind of a "revolution". But, if you don't have a problem with that, I am more then happy to go that way.

For example, let's say we want to test how RUA works internally. Our "written in English" test would sound, I assume, smth like: "whenever a package is installed, a call to remove the build directory should be made". In terms of traits and mocks, it should be a "package installer" function that accepts a "directory deleter" trait, and calls it when required. Now we want to test the "package installer" function, and instead of giving the function a real "directory deleter", we would provide a fake one. We would then ask the "package installer" to proceed. The mock would tell us if "directory deleter" was indeed called in the process, and possibly check which arguments were used.

Understood and agreed.

Going further into the dependency tree, the function that uses "package installer" could also take "package installer" as a trait, and in testing we could override that as well. That would be the full DI / testing approach.

That seems like a very big step for a single PR/branch. Maybe we should leave it for the future and/or a different PR that will be opened after that one is finished?

Now we're getting to the problem. In this PR, "package installer" does not really accept a "directory deleter" trait. We're abstracting away from "directory deleter" by using a trait, but we can only ever use one specific "directory deleter", which is hardcoded in the current PR in line 60 of actions_install.rs: https://github.com/vn971/rua/pull/146/files#diff-863878877eeb9159c0270d8aa71de01d3281a4b761037e67af3430957626977fR60 We didn't test "package installer"-s behavior, we only tested a self-contained "directory deleter" behavior.

Agreed. Again, I was afraid of making a "revolution".

That's the problem that I saw, I'll write a separate response on where the code could go from there

refaelsh commented 3 years ago

Hope any of that helps...

It does! A lot! Thank you very much :-)

refaelsh commented 3 years ago

The solutions on how to address the problem, as far as I see, could be:

* Just use a direct call to `rm_rf` to remove the directory. We could then proceed with the actual feature of `rua clear-cache` and such. The imperative code for package installation would then remain untested as far as automated tests are concerned.

* Make "package installer" accept a "directory deleter". Said "package installer" should then be testable in actual automated tests. I'm not actually very convinced on what the point of the test would be. If the test would be to check that certain actions are called in certain order, then it would kinda literally repeat the code. The function at hand doesn't seem like a parser or any such function with a broad spectrum of possible arguments and broad spectrum of possible things that the function could do. I'm a big fan of testing, but only when we test something that is really different from literally repeating the code 1:1.

Note that if you'd like to proceed with the second approach, some other things would be good to fix:

Yes, I would like to proceed with the second solution.

* Think if we really need the `FolderDeleter` and `LeftOversDeleter` traits? It seems that a function `path: &PathBuf -> rm_rf::Result<()>` could replace the trait (or itself being a trait)? Or if "named traits" are preferred over "anonymous function", then maybe keep one of the traits since this is what we're actually interested in when we check if "package installer" works correctly?

LeftOversDeleter is not a trait, its a concrete class that accepts a trait as a ctor parameter.

* Note sure what `m_` denotes here, could be improved if we go for this approach: https://github.com/vn971/rua/pull/146/files#diff-3e1fe2772b5f94ba3578d10793d98ab1cd6ee248143221847a11dc9ded130e19R6

m_ is from the word member, as in data member of the class. Its a common (?) convention. I really like it. IMHO, it improves readability of the code. Are you not familiar with that convention?

* Since the function returns a `Result` already, `unwrap()` at line 21 should be re-written to return actual values: https://github.com/vn971/rua/pull/146/files#diff-3e1fe2772b5f94ba3578d10793d98ab1cd6ee248143221847a11dc9ded130e19R21

I don't understand.

* Panicking inside tests could be acceptable I guess, but it'd be still nice to provide written description for the reason of failure and why it's unacceptable for the test. This goes for the second `unwrap()` beind used and `assert_eq!`: https://github.com/vn971/rua/pull/146/files#diff-3e1fe2772b5f94ba3578d10793d98ab1cd6ee248143221847a11dc9ded130e19R54

I will add it if you want to. But, from my experience, when I get an email from CI/CD server, eventually I anyway get my hands dirty to investigate why the unit test failed, so the failure message does not really matters.

refaelsh commented 3 years ago

If the test would be to check that certain actions are called in certain order, then it would kinda literally repeat the code.

Yea, good point. I will take it under advisement.

refaelsh commented 3 years ago

@vn971 I am done with what you suggested and I feel better :-) The "revolution" is not that big as I feared.

P.S. But I must note that I was not able to write unit tests for the "package installer" as you suggested (but I am OK with that). The function depends on things that are not abstract interfaces and therefor can not be mocked for unit testing.

vn971 commented 3 years ago

To give an overall comment while I see you're making commits in git. The most valuable places to cover with tests should probably be those where the logic flow itself is non-trivial. E.g. if the code is a step-by-step direct execution, then there's not much to test there except to repeat the same sequence. If the code loops back and does weird things like that, it's a good candidate to mock what it touches and test that the order is good.

For example, recursive package download recursive_info could be tested to make few requests with no extra unneeded packages. (Although here I'd feel a little bit guilty at not accepting Morganamilo-s PR back in the days, but the approaches were really different there.)

Otherwise I don't actually see places that wouldn't directly say what happens and how TBH. It's all really "declarative" code visually and maintenance-wise, even though technically "imperative" all the way through.

vn971 commented 3 years ago

If the aim is to improve code quality in general, then CI would be a very attacking point to my eyes as well. One good approach could also be to reduce the usage of panic/unwrap (I'm a bit guilty of doing that 2 years ago) and use Result. This could unblock us to e.g. do https://github.com/vn971/rua/issues/148

refaelsh commented 3 years ago

while I see you're making commits in git

No no, that is it, I am done. Ready for you reviewing pleasure :-)

vn971 commented 2 years ago

Hey! I think I'm going to close the PR, it overall seems to increase the conceptual complexity (e.g., number of concepts) of the project without giving a lot of the benefits that this could introduce. I'll leave more comments to the exact files around

refaelsh commented 2 years ago

Thank you very much for your time @vn971 :-) I've had lots of joy reading your remakes. I've learned a lot from them.

refaelsh commented 2 years ago

Meaning that the code literally says in which order to execute which actions. Tests are not as good for testing such behavior as they would literally repeat the very same things.

I am now at a phase of my professional career where I understand what you are saying here. It was not the case when I opened the PR :-)

refaelsh commented 2 years ago

That's my subjective opinion

I agree here, code that has no ifs and/or fors and/or whiles should not be unit tested due to what you said: literally repeat the very same things. This is similar to what Martin Fowler calls the Humble Object pattern, and he clearly states this pattern does not requires Unit Tests (most of the time anyway).