Added unit tests and Github work flows to run tests and lints #10

Mart-Bogdan · 2022-12-16T23:05:37Z

Lints would post all clippy warnings as annotation to pool request

Mart-Bogdan · 2022-12-16T23:53:48Z

By the way. It should add tests count as comment to PR, like here: Mart-Bogdan#3 (comment)

I guess it is because it is not merged yet. Or because I am not a maintainer.

Mart-Bogdan · 2022-12-16T23:55:47Z

By the way. It should add tests count as comment to PR
I guess it is because it is not merged yet. Or because I am not a maintainer.

Oh. That's unfortuante:

Publish Unit Tests Results
This action is running on a pull_request event for a fork repository. It cannot do anything useful like creating check runs or pull request comments. To run the action on fork repository pull requests, see https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#support-fork-repositories-and-dependabot-branches

.github/workflows/check-and-lint.yaml

.github/workflows/test.yaml

Mart-Bogdan · 2022-12-23T23:30:43Z

Hi, @scullionw.
I've addressed commets, and removed dead code in actions.

Would there be other comments regarding tests?

Sorry for slow reaction time. I have issues with internet and electricity, and also some chores.

Mart-Bogdan · 2022-12-23T23:33:21Z

I actually don't like to put actual files into repo, but I wasn't able to come with anything more clever.

Perhaps generating them as random files with predefined seed, hm.

Random files are mostly needed to work with compressed files.

Mart-Bogdan · 2023-01-10T13:22:49Z

Hello, @scullionw. Happy holidays. Happy new year.

So what about this PR?)

scullionw

cool, thanks! lgtm once we address a few things

Cargo.toml

scullionw · 2023-02-03T13:36:25Z

src/tests.rs

@@ -0,0 +1,225 @@
+//! Be aware that tests are run in parallel. For this reason we must be sure to use


since we are testing the public api, can we move the tests outside src/ to be an integration test? ie tests/pathlength.rs tests/sizes.rs etc?

see https://doc.rust-lang.org/book/ch11-03-test-organization.html#integration-tests

Need to take look at your link. And about pros/cons

For example in branch for windows, i also used private function call in tests: ffi::set_file_compression(file.path(), compress)?;

https://github.com/Mart-Bogdan/dirstat-rs/blob/max-path/src/ffi.rs#L126-L169

https://github.com/Mart-Bogdan/dirstat-rs/blob/max-path/src/tests.rs#L248

But generally, looks like you are correct. This are actually integration tests. And they interract with external world.

So looks good. Shall try doing it.

Shall test-data dir go under tests dir as well?

scullionw · 2023-02-03T13:36:50Z

src/tests.rs

+
+use crate::{DiskItem, FileInfo};
+// warn: don't remove `as &str` after macro invocation.
+// It breaks type checker in Intellij Rust IDE


i'd prefer not adding IDE specific notes to the source

well. Perhaps this one is not needed. I think as &str thing is already fixed upstream in const_format crate. I shall remove this

scullionw · 2023-02-03T13:37:10Z

src/tests.rs

+99999999/999999/99999999/99999999999/99999999/999999999/9999999999/";
+
+const PATH_1_FULL: &str = concatcp!(TEST_DATA_DIR, LONG_PATH_DIR, PATH_1) as &str;
+//noinspection SpellCheckingInspection


same, I think you can disable spellcheck in your idea instead

You aren't using spellchecker plugin in vscode? Or whichever IDE you are using.

I think disabling spellchecker altogether is harmful.

It's useful in other places. And I sometimes have problems with spelling :-(

I don't think it's such a big deal, having this comment. We can add second one for VSCode spell checker as well :-)

scullionw · 2023-02-03T13:39:59Z

src/tests.rs

+    let result = FileInfo::from_path(test_path, true);
+
+    // Then
+    if let Result::Ok(FileInfo::Directory { volume_id }) = result {


this is equivalent to `expect("Can not get file info)",

also "Ok" is already in scope, no need for "Result" :)

Yeah, you are right about Result.

About expect, just tried it. Don't work :-( it only covers Ok but don't allow to match Directory

the error is:

--> src/tests.rs:63:9 | 63 | let FileInfo::Directory { volume_id } = result.expect("Can not get file info"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pattern `File { .. }` not covered | = note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant

So anyway if let would be needed.

scullionw · 2023-02-03T13:45:29Z

src/tests.rs

+fn test_file_size_8KiB() {
+    const DIR: &str = concatcp!(TEST_DATA_DIR, "test_file_size/") as &str;
+    // do not rename it into `_` it would cause immediate destrucion after creation
+    let _guard = CleanUpGuard { path: DIR };


this is an anti-pattern IMO. if we want to rely on drop to cleanup, we should need to use the type itself. (like create_dir taking a &GuardedPath as argument or something). easy to forget otherwise!

I've eused pattern mentioned here: https://stackoverflow.com/questions/38253321/what-is-a-good-way-of-cleaning-up-after-a-unit-test-in-rust

Unfortunately I do not know better way of doing something after code is executed. Regardles if panic was raised.

Somebody are proposing setting panic hook, but that looks like dirty hack.

std::panic::set_hook(Box::new(|_| { cleanup();

What do you mean create_dir should take Guard as argument? Won't this cause it to clean-up straight after function call?

But ok. I can think on it after other items are resolved. Perhaps you would be able to come with something better for clean-up.

I'll move all the code into integration tests first, I think.

scullionw · 2023-02-03T13:48:43Z

do all tests pass without the other PR? if not lets mark them as #[should_panic], and then remove them in the other PR, so that each PR by itself is mergeable

Mart-Bogdan · 2023-02-08T11:33:45Z

do all tests pass without the other PR? if not lets mark them as #[should_panic], and then remove them in the other PR, so that each PR by itself is mergeable

I did not include tests that won't work without other PR. Besides, they won't compile. They are still in separate branch.

Mart-Bogdan · 2023-02-08T11:41:01Z

Ah. shuch tests currently are marked as #[cfg(not(windows))] hmm.

Mart-Bogdan · 2023-02-22T18:59:05Z

Hello, @scullionw, regarding integration tests.

I'm thinking. This crate exports library as well as executable. Is it required, and do we need to keep backward compatibility for lib?

Mart-Bogdan mentioned this pull request Dec 16, 2022

Rework windows "Physical size" calculation, added Unit Tests #9

Open

Added unit tests and Github work flows to run tests and lints

d1d5527

Mart-Bogdan force-pushed the unit-tests branch from 5412669 to d1d5527 Compare December 16, 2022 23:44

remove deprecated github actions

09fef3e

scullionw reviewed Dec 18, 2022

View reviewed changes

.github/workflows/check-and-lint.yaml Outdated Show resolved Hide resolved

.github/workflows/test.yaml Outdated Show resolved Hide resolved

Mart-Bogdan mentioned this pull request Dec 23, 2022

Fix clippy warnings and add Cargo-clippy check to CI #12

Open

Addressed PR comments. Removed not working CI steps.

c208283

Mart-Bogdan force-pushed the unit-tests branch from 7aabf8a to c208283 Compare December 23, 2022 23:26

scullionw approved these changes Feb 3, 2023

View reviewed changes

Resolve some of PR comments. Not all yet.

803324e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added unit tests and Github work flows to run tests and lints #10

Added unit tests and Github work flows to run tests and lints #10

Mart-Bogdan commented Dec 16, 2022

Mart-Bogdan commented Dec 16, 2022

Mart-Bogdan commented Dec 16, 2022

Mart-Bogdan commented Dec 23, 2022

Mart-Bogdan commented Dec 23, 2022

Mart-Bogdan commented Jan 10, 2023

scullionw left a comment •

edited

Loading

scullionw Feb 3, 2023

Mart-Bogdan Feb 8, 2023

scullionw Feb 3, 2023

Mart-Bogdan Feb 8, 2023

scullionw Feb 3, 2023

Mart-Bogdan Feb 8, 2023

scullionw Feb 3, 2023

Mart-Bogdan Feb 8, 2023

scullionw Feb 3, 2023

Mart-Bogdan Feb 8, 2023

Mart-Bogdan Feb 8, 2023

scullionw commented Feb 3, 2023

Mart-Bogdan commented Feb 8, 2023

Mart-Bogdan commented Feb 8, 2023

Mart-Bogdan commented Feb 22, 2023

		@@ -0,0 +1,225 @@
		//! Be aware that tests are run in parallel. For this reason we must be sure to use

Added unit tests and Github work flows to run tests and lints #10

Are you sure you want to change the base?

Added unit tests and Github work flows to run tests and lints #10

Conversation

Mart-Bogdan commented Dec 16, 2022

Mart-Bogdan commented Dec 16, 2022

Mart-Bogdan commented Dec 16, 2022

Mart-Bogdan commented Dec 23, 2022

Mart-Bogdan commented Dec 23, 2022

Mart-Bogdan commented Jan 10, 2023

scullionw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scullionw commented Feb 3, 2023

Mart-Bogdan commented Feb 8, 2023

Mart-Bogdan commented Feb 8, 2023

Mart-Bogdan commented Feb 22, 2023

scullionw left a comment •

edited

Loading