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

Allow writing a single test case which will scan the current classpath for all @Immutable classes #44

Open
Grundlefleck opened this issue Aug 27, 2013 · 16 comments

Comments

@Grundlefleck
Copy link
Contributor

@Grundlefleck Grundlefleck commented Aug 27, 2013

Currently, if you want to start using Mutability Detector on your project, and you already have a lot of classes marked as @Immutable, you can either:

  • use the FindBugs plugin to automatically detect them, in which case, you can't configure the assertion to suppress false positives
  • add a unit test for every class, in which case, you have to spend a lot of tedious time writing new test methods.

I would like to find a solution which offers the best of both worlds: to be able to quickly check all your immutable classes, but which still allows flexible configuration for suppressing false positives.

Currently I think we could do something where:

  • a user configures one assertion in a unit test
  • the test scans the classpath for @Immutable classes and analyses them, reporting errors from all classes
  • ... excluding classes which are being tested in another method, with custom suppressions.

This could possibly use a library like http://scannotation.sourceforge.net/ or something similar, to do the scanning. I don't know yet how this method could exclude classes which are being tested elsewhere. Perhaps we would need to get creative.

@Grundlefleck
Copy link
Contributor Author

@Grundlefleck Grundlefleck commented Sep 19, 2013

@MaximumTrainer
Copy link

@MaximumTrainer MaximumTrainer commented Jul 7, 2014

Well its been a while as I've been settling into my new job and teaching basic TDD. I now have a need for this and will start looking into a solution

@neomatrix369
Copy link
Member

@neomatrix369 neomatrix369 commented Jul 7, 2014

Sounds like a good plan! Looking forward to your participation.

On Mon, Jul 7, 2014 at 9:01 PM, sandeater notifications@github.com wrote:

Well its been a while as I've been settling into my new job and teaching
basic TDD. I now have a need for this and will start looking into a solution

@lasombra
Copy link
Contributor

@lasombra lasombra commented Dec 1, 2015

That is a very interesting issue. I know nothing about class scanning but I'm interested in giving this a try. Is that OK?

@Grundlefleck
Copy link
Contributor Author

@Grundlefleck Grundlefleck commented Dec 1, 2015

@lasombra absolutely!

I know nothing about classpath scanning either, so hopefully we can learn together 😄

This entire issue may be a bit big as it's currently described. So I think a good contribution to get started would be a change to Mutability Detector where a method was available, which, when invoked, scanned the classpath and lists all the classes it finds annotated with @Immutable. Once we have that we can then start to talk about actually asserting on them, customising the assertions, etc. If you want to start even smaller, and just find something really tiny, like fixing a typo in the documentation, just to get into the swing of forking the project and issuing pull requests, that's cool too.

I see you're on the LJC's Slack. Feel free to ping me there for any questions you have in getting the project checked out, built. I don't know your level of familiarity with Java/Maven/Git etc, but no question is too basic!

lasombra added a commit to lasombra/MutabilityDetector that referenced this issue Dec 1, 2015
lasombra added a commit to lasombra/MutabilityDetector that referenced this issue Dec 2, 2015
lasombra added a commit to lasombra/MutabilityDetector that referenced this issue Dec 2, 2015
* origin/master:
  Initial work on MutabilityDetector#44.

# Conflicts:
#	src/main/java/org/mutabilitydetector/classpath/ClassPathScanner.java
Grundlefleck added a commit that referenced this issue Dec 4, 2015
Implementing #44
@sband
Copy link
Contributor

@sband sband commented Mar 23, 2019

Is this issue resolved yet ? if there is any work remaining around this and no one can pick up, please let me know. I could look into it

@Grundlefleck
Copy link
Contributor Author

@Grundlefleck Grundlefleck commented Mar 23, 2019

@sband
Copy link
Contributor

@sband sband commented Mar 28, 2019

ok, cool. For scanning the classes should we look at this open source project https://github.com/classgraph/classgraph ? it is compatible with Java 9 as well.

@Grundlefleck
Copy link
Contributor Author

@Grundlefleck Grundlefleck commented Mar 28, 2019

My ideal solution would:

  • not require classes to be loaded. Anything using reflection (e.g. java.lang.Class et al) requires classes to be loaded. I'd prefer to just analyse .class files and their bytecode, without worrying about having to load the class. This is mostly to avoid excess memory usage (in PermGen/Metaspace) that I've found difficult to manage in code.
  • be fast. I'd like to not have to do something like persist an index across runs, because of the extra effort and complexity to manage the state across different JVM lifetimes. But I'm not ruling it out if that's what it takes to be fast.
  • not have false negatives. It needs to find 100% of classes annotated with @Immutable, even weirder cases like inner classes, because these are perhaps the most likely to also be mutable. Any scanning, whether custom written, or using a library, should be able to find these classes by default.

@sband with that as a rough guidance, I'll let you investigate and make a judgement call on whether classgraph is suitable or not. How does that sound?

@sband
Copy link
Contributor

@sband sband commented Mar 29, 2019

Awesome ! Most of the requirements you mentioned are addressed in classgraph. Here is the link for reference - https://github.com/classgraph/classgraph/wiki/How-fast-is-ClassGraph%3F
Now what remains is the problem about false negatives which i think can be sorted out through testing. I think classgraph is a suitable solution for scanning part of the problem.

I think i am asking too much, but would it be possible for you to explain the other part of this problem in more detail, i.e. creation of API so that i could proceed accordingly. An example would be more useful.

@ShreyasKudari
Copy link

@ShreyasKudari ShreyasKudari commented Oct 15, 2020

Is this issue still open? If yes I'd like to start looking into this! Please let me know.

@Grundlefleck
Copy link
Contributor Author

@Grundlefleck Grundlefleck commented Oct 16, 2020

@ShreyasKudari yes, still open. We don't have this functionality yet and I still believe it would be a fantastic feature.

Let me know how I can help you get started. Feel free to issue small pull requests / fixes to get into the flow of committing. You don't have to have everything finished and perfect before contributing. I'd actually encourage the opposite, small, iterative improvements if you can. Thank you for your interest and don't hesitate to reach out.

@ShreyasKudari
Copy link

@ShreyasKudari ShreyasKudari commented Oct 17, 2020

Got it. I will study this over the weekend and update you if I need something clarified.

@erikhofer
Copy link

@erikhofer erikhofer commented Feb 24, 2021

What do you think about integrating with ArchUnit? I think it would be way more flexible and you don't have to worry about classpath scanning.

@Grundlefleck
Copy link
Contributor Author

@Grundlefleck Grundlefleck commented Feb 26, 2021

@erikhofer
Copy link

@erikhofer erikhofer commented Feb 26, 2021

@Grundlefleck I'm neither affiliated nor an expert. I think you should not wrap it but provide an ArchRule so that users of ArchUnit can simply include it. Take a look here: https://github.com/TNG/ArchUnit/blob/main/archunit/src/main/java/com/tngtech/archunit/library/GeneralCodingRules.java

Probably would look something like

classes.that().areAnnotatedWith(Immutable.class).should(BE_IMMUTABLE)

where BE_IMMUTABLE is a custom ArchCondition that wraps MutabilityDetector. ArchUnit exposes a low-level API for implementing custom rules. See https://www.archunit.org/userguide/html/000_Index.html#_creating_custom_rules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants