Add a simple test.

[tompscanlan]

Adding this to open discussion around the issue #7 , and to lay framework for fixing #97

[tbillington]

I like the direction of this! Thank you for having a crack.

A few points of feedback :)

What you're doing would make more sense to me as an integration test of kondo_lib. So it would go in kondo_lib/tests and probably call scan. Needing to go through discover from kondo is quite awkward.

I'd also suggest test utilities to create the test projects as opposed to having them hardcoded in the repo, like you've done in _create_fake_python_project. You can see a basic example of this in use by the homebrew install code.

Avoid referencing a absolute path that is based on your home directory.

[tompscanlan]

Thanks, I use tests to help understand what's going on, and it grew from main, so I ended up testing from the wrong place.

I'm really trying to isolate parts, so I'll stick to the unit test model, but relocate to the right module.

I'll iterate and ping you next time.

[tompscanlan]

ok... done with changes for a few hours @tbillington. Thoughts?

[tbillington]

This is looking good so far, appreciate you iterating on it. Adding tests for the first time is always a bit painful :)

There's really only a few blockers for me

• As I mentioned in the test.rs comment, specifying the test directory structure in code is strongly preferred
• I hadn't realised lints weren't applying to tests, I've updated the CI job so please rebase/merge with master to get that (and some example tests in kondo_lib)
• Moving the tests to kondo_lib is strongly preferred to avoid changing the public api, let me know if you'd like more specific info on this

I'd also suggest you enable clippy in your editor of choice if possible, it's a great rust learning tool! If you're using vscode you just need to add "rust-analyzer.check.command": "clippy", to your settings.json.

If you have any questions don't hesitate to ask

[tompscanlan]

Hey @tbillington,

Thanks for the feedback. I'm still really new to rust, so I haven't used clippy. I'll look into it. Thanks.

re: Specify test in kondo-lib
I'm intending to test discover and clean of kondo, not kondo-lib. Since those live in kondo, I'm planning on keeping tests in kondo. I can add some tests for kondo-lib. I'm feeling the resistance here, but I'm pretty keen on keeping these. Want to hop in files review or a quick screen share session to mark up parts that really bug you?

re linting, I'll refresh today (I'm US east coast, so we're dominating the 24 hour clock).

re changing public api.
I'm not following where I'm changing the public api. Can you point at something that changed to a user? Splitting kondo/main.rs into kondo/lib.rs and main.rs could be doing something I'm not aware of.

I'll look closer at kondo-lib today and see if what you say sinks in. Will make another iteration and see your feedback tomorrow.

[tompscanlan]

Ok... think I'm following on where I was testing from kondo into the lib for clean(). I moved that kondo-lib, but still seeing need for the discover() test to remain in kondo.

Peek at this revision and let me know.

[tbillington]

Apologies, I'd written comments but forgot to "submit" the review !

It's a bit late here atm so I'll check back later :)

[tbillington]

The diff of this file is a bit unfortunate, wish git would understand what's happening better :/

[tompscanlan]

+1 the gist is I'm slicing it into exe and lib parts. I think I got it right, but check the real files, cause the diff is gross :)

[tbillington]

I much prefer this method of constructing test directories.

kondo currently supports 16 project types, and if we want to have multiple variations per type to test different cases.. this repo will grow quite a bit :). It also couples strings (paths) in test code with folder structure unnecessarily.

I don't expect you to add support for any more than you're already doing, just thinking ahead.

[tompscanlan]

I'm torn.. think that's why I came at it both ways.

I like creating the test dir programmatically for unit tests, but see some value in a user dropping you a tarball containing a project that doesn't work, and us including a distillation of that as a test scenario. Think regression testing.

[tbillington]

I think it wouldn't be too hard to quickly re-create that via a code method, especially with some helper functions.

Another option which I previously hadn't considered was specifying them in something like json files, haven't thought much on it though.

[tompscanlan]

I've kept both for now.

lib unit tests: create_fake_python_project . I'm not sure how to share this code across the two modules' unit tests, so it's currently duplicated here. I don't like this :) what do you recommend?

integ tests: with_temp_dir_from directory based scenarios

[tompscanlan]

Review what's here at your leisure once more and I'll make another pass before rebasing again.

A part I'd like to change. but I'm not sure if I'll get to today... is to move the programmatically created project into the unit test, and the file-based test out to the integ/cli test. Including your wrapper syntax around temp dirs.

[tompscanlan]

I think I got to almost all your feedback. I like the wrapper idea and played with it. Ready for feedback.

[tbillington]

Sorry I haven't gotten to this yet, been a bit busy.

You mentioned being open to chat, just curious which timezone you're in? I'm in GMT+10

[tbillington]

Re-formatting, I have this in my settings.json

  "[rust]": {
    "editor.defaultFormatter": "rust-lang.rust-analyzer",
    "editor.formatOnSave": true
  },

[tompscanlan]

I'm GMT-4. My night/your morning to midday are the best opportunities. I'm also a bit tied up, but I'll get to this this week.

[tompscanlan]

This should pass the ci, at least :)

Open questions I have:

1. what to do about duplicated unit test code for creating the fake project. Ignoring it is an option. I think pushing it into a test module and calling that from both tests might work.

2. docs? Want anything in the readme?

3. I've got a couple test cases that were failing so I "ignored" them. I might be mistaken or misunderstanding, so take those with a grain of salt.

4. re chatting. I can be open around 930pm GMT-4 some time. Do you have a favorite coms channel to use?

[tbillington]

Sorry for the slow reply on this Tom, had a few things on my plate but it's on my list to get around to this hopefully this week.

[tompscanlan]

Not a problem. PRs can wait, take care of what you need.

[tompscanlan]

adding a few tests around path_canonicalize(). Seems like there is a bug there when --ignored-dirs contains dirs that don't exist, and aren't absolute.