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

Circular dependencies in test suites #10

Closed
zhming0 opened this issue Jul 30, 2023 · 9 comments
Closed

Circular dependencies in test suites #10

zhming0 opened this issue Jul 30, 2023 · 9 comments

Comments

@zhming0
Copy link

zhming0 commented Jul 30, 2023

In Clojure, tests are conventionally located in prefix.ns-test, it post a small challenge in using init. Because for example, if I use init to scan namespace prefix company, it would normally auto require all test case namespaces to form system graph, however, those integration test cases would normally need to import the system graph, which results in circular dependencies.

flowchart LR
    root[company.main] -->|Scan| test(company.main-test)
    test -->|need system graph| root 
Loading

If I want to keep the clojure test namespace naming convention, there seems to be no good workaround for now. My current workaround leverage runtime (resolve) + kaocha hooks, but it's a bit ugly.

I wonder if it's worth to have a granular control when init do scan, perhaps, support filtering during scan? For example, we can allow pass in a filter function to filter out certain namespaces, such as xxxx-test.

@ferdinand-beyer
Copy link
Owner

Hey @zhming0,

Thanks for reporting!

I wonder how you are scanning your namespaces? I try to keep init very modular, so that you can use the building blocks if the built-in tools do not do what you need.

For classpath scanning, init.discovery/scan combines searching, loading, and building a configuration. If you want to filter before loading, you can create your own scan function, e.g.:

(let [ns-names (->> (init.discovery/classpath-namespaces "company")
                    (remove #(str/ends-with? (name %) "-test")))]
  (run! require ns-names)
  (init.discovery/from-namespaces (map the-ns ns-names)))

Another option is to keep company.main super slim, so that you don't need to test it. My main namespaces usually contain little else then the -main function, which does not make sense for me to write tests for. Anything else can go to different namespaces, so that nothing depends on my main namespace.

I often also use a different name for the main namespace, e.g. company-project, so that it does not match the classpath scanning prefix. Otherwise we would end up require-ing ourselves in init.discovery/scan, which is usually not a problem, but might lead to weird behaviour, as the company.main namespace would not be fully loaded yet.

Maybe that works for you as well?

@zhming0
Copy link
Author

zhming0 commented Jul 31, 2023

Thanks for the reply @ferdinand-beyer!

I normally scan my namespaces via (init/static-scan 'my-company). The custom scan option is valid, but that would also mean that I basically have to create two scans as well, one static for uberjar build, one dynamic for REPL. 🤔 I wonder if adding an optional param filter-fn to init's scan and static-scan will be a good option?

The option of keeping company.main slim isn't going to work in my case. I don't have test cases for company.main but my company.service.foo-test will need to require the system graph so it can spin up a test system, that's why they will require company.main.

The last option of using difference name for main namespace doesn't seem to address the circular dependency either, company-project ns will require all tests to form system graph but test namespaces will require system graph to function. so it's the same error I guess.

So it sounds to me that option 1 is the only valid option and now the problem is whether it's a good idea to get init to support this feature. Is there a way to add such a feature without compromising modularity?

@ferdinand-beyer
Copy link
Owner

ferdinand-beyer commented Aug 5, 2023

my company.service.foo-test will need to require the system graph so it can spin up a test system

I think this is the reason for your circular dependency. I normally don't do that, but instead write unit tests for components without spinning up a system. Since every component takes its dependencies as function arguments, this is quite straight-forward, and you can also pass test doubles as needed.

Since you spin up a test system, I assume you have sort some of integration test, and I see the value of testing the system that is actually built. For test systems, you often might want to replace some components with test doubles as well.

In your case a simple solution would be to put these tests in a different namespace, like company-test. A (static-scan 'company) would then pick up unit tests like company.component-test, that should not require a test system, but not company-test.integration-test.

You will still have a circular dependency if you scan for company in company.main though (company.main requires itself). A cleaner approach might be to only scan sub-paths, e.g. have a main namespace company.project, an integration test in company.project-test, but put all your components in namespaces with the prefix company.project, like company.project.component. Note that scanning for the prefix company.project will not pick up company.project itself, as it looks for files company/project/** in the classpath.

Having said all that, I think adding a way to customise scanning might indeed be useful...

@ferdinand-beyer
Copy link
Owner

Added include? and exclude? options in v0.2.94.

You can now do:

(discovery/static-scan '[company] :exclude? #(str/ends-with? % "-test"))

@zhming0
Copy link
Author

zhming0 commented Aug 6, 2023

Thank you for taking care of this! Really appreciated! 🙇🏿 🙇🏿

@zhming0
Copy link
Author

zhming0 commented Aug 7, 2023

Hmmm, for some reason that I don't understand yet, although classpath-namespaces seems to work properly, when I do static-scan, it still requires those excluded namespaces.

Then I tried to simplify it:

(discovery/static-scan '[company] :exclude? (constantly true))

I got compiler error instead: java.lang.ClassCastException.

@ferdinand-beyer
Copy link
Owner

Argh, that's probably because of macro expansion...

Since static-scan is a macro, it will pass {:exclude? '(constantly true)} to emit-static-scan. (constantly true) will not be evaluated, so it is a list of a symbol and a boolean, and not a function.

This will lead to: class clojure.lang.PersistentList cannot be cast to class clojure.lang.IFn.

I forgot to (eval) this :(

Even then, we need to make sure that we actually find something, or emit-static-scan will emit an empty (require).

@ferdinand-beyer
Copy link
Owner

@zhming0 -- can you try v0.2.96 please?

@zhming0
Copy link
Author

zhming0 commented Aug 8, 2023

@ferdinand-beyer v0.2.96 works! Thanks a lot!

@zhming0 zhming0 closed this as completed Aug 8, 2023
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