Unit tests for the JctFileManagerImpl class

[ascopes]

Write unit tests for the JctFileManagerImpl class

[Badrri-Narayanan]

Hi, I would like to work on this issue, can you please assign this to me?

[ascopes]

Sure thing!

[ascopes]

@Badrri-Narayanan feel free to take your time and shout if you need anything! Also, feel free to do this in multiple PRs if that is any easier for you!

[Badrri-Narayanan]

@ascopes sure thanks

[Badrri-Narayanan]

Hi @ascopes , I forked the project and cloned it to my local. But when I try to import it as maven project after installing Groovy IDE integration for Eclipse, I am getting the following error

Detailed stack trace:

Errors occurred during the build.
Errors running builder 'Java Builder' on project 'acceptance-tests-spring'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-serviceloader-jpms'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-serviceloader'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-micronaut'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-mapstruct'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-manifold-systems'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-lombok'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-immutables'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-google-auto-value'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-google-auto-service'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-google-auto-factory'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-error-prone'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-dagger'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-checkerframework'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-avaje-jsonb'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory
Errors running builder 'Java Builder' on project 'acceptance-tests-avaje-inject'.
org/codehaus/jdt/groovy/integration/LanguageSupportFactory

Also, I am new to groovy and I am an exprienced Java developer. Can you please let me know I can run the project? Thanks in advance

[ascopes]

not entirely sure on the issue here unfortunately. It looks like an issue with Eclipse loading groovy.

You shouldn't need to run the acceptance tests as part of this though. You could always exclude them from the project.

Under IntelliJ IDEA, it seems to load fine, I don't use eclipse unfortunately though so I am not entirely sure.

I use groovy in the acceptance tests as it removes a bunch of complexity around reflection for some of the stuff I am testing against.

The groovy stuff should just run, since the versions of stuff are defined in the POM.

[Badrri-Narayanan]

ok @ascopes , I will try it in Intellij. Thanks for the suggestion!

[ascopes]

No problem! I use the latest version via JetBrains Toolbox (https://www.jetbrains.com/toolbox-app/) if you want to match the version I am developing with.

Will open an issue regarding eclipse to see if I can look into it at some point and add some instructions in the Wiki towards setting it up with this project.

Edit: #226

[Badrri-Narayanan]

It's working in Intellij! Thanks!!

[Badrri-Narayanan]

Hi @ascopes , I have added two Junit testcases for filemanager. Kindly let me know if I am proceeding in the right direction.

Also, I tried adding testcase for addPath method but I am having trouble creating an instance of ModuleLocation as the parent is also of type Location (with ModulleLocation being concrete implementation of Location. I am facing circular dependency). Your feedback will be much appreciated. Thanks in advance!!

[ascopes]

So for ModuleLocation, the JDK should have a class StandardLocation in the java.compiler module that should be used as the parent for ModuleLocation (e.g. StandardLocation.MODULE_SOURCE_LOCATION). The use case is representing a specific module in a location that can contain zero or more modules..

Failing that, you can always mock the object if you dont need a concrete implementation (e.g. Mockito.mock(Location.class)). I tend to mock dependencies if it isn't simple to initialise them as it reduces the number of dependencies on other classes unnecesarilly. Integration between classes is then tested in integration tests instead, although I haven't got as far as writing those just yet.

For the changes you have done, would you mind adding a link if it isn't in a PR (on mobile right now, GitHub doesn't always send me notifications properly on mobile for some reason).

[ascopes]

One thing to note, in this I have used AssertJ rather than Junit assertions, so it is probably a good idea to use those (if you look at, say, the compiler impl tests, you'll see examples of how I have done this and how it is structured to test each method).

https://github.com/ascopes/java-compiler-testing/blob/main/java-compiler-testing/src/test/java/io/github/ascopes/jct/tests/unit/compilers/impl/JctCompilationImplTest.java

Hope that helps a bit!

[Badrri-Narayanan]

Thanks @ascopes . Sure, I will refactor the code as advised.

[ascopes]

@Badrri-Narayanan heyo, are you happy and able to continue implementing the rest of the tests for this class/package?

If you are busy or do not want to, then that is no problem!

If you do, and fancy starting with something simpler, a simple one to start off with might be the PathFileObject. That is relatively small and only does a few specific things.

There is also some methods in the io.github.ascopes.jct.test.helpers.Fixtures class to create some mocks and stubs of commom objects you might need in the tests. If there is anything common you are mocking and configuring, feel free to add stuff to that class :-)

Thanks for the help so far! Appreciate the codebase takes a while to understand since there is a lot of complex stuff going on there. I might try and add some stuff about the architecture to the Wiki at some point to help with this.

[ascopes]

I'll also see about adding you and @sohel-79 to the allowlist for the CI pipelines so you do not have to wait for permission to run the CI pipelines on PRs.

Edit: looks like since you have made your first PRs, it shouldn't wait for my approval to run CI now.

If you have any queries on anything (either of you), feel free to start a discussion on the discussions page, too :-)

[Badrri-Narayanan]

Thanks @ascopes . I am still trying to get my head around the codebase and some info related to architecture would really help. I also noticed something while writing tests for JctFileManagerImp module and I wanted to know your thoughts.

In the following method, the if-else statements are doing the same thing, can we simplify this?

  @Override
  public void addPath(Location location, PathRoot path) {
    if (location instanceof ModuleLocation) {
      var moduleLocation = (ModuleLocation) location;

      if (location.isOutputLocation()) {
        getOrCreateOutput(moduleLocation.getParent())
            .addModule(moduleLocation.getModuleName(), path);
      } else {
        getOrCreateModule(moduleLocation.getParent())
            .addModule(moduleLocation.getModuleName(), path);
      }

[ascopes]

Okay, so, in that specific example, the getOrCreateModule method uses a separate mapping to the getOrCreateOutput. This gets a little complex in why it has to work like this, so I'll try and give an explaination.

With Java, there is the java.compiler module which is a set of interfaces for implementing and controlling Java compilers (like javac, ECJ which Eclipse uses, etc). JCT uses these interfaces to build new versions of some components like diagnostic listeners (captures everything that the compiler outputs, like error messages and warnings and stuff); and file managers (which are an API the compiler uses to read and write files in various places).

Java compilers use the concept of "locations" which are considered to represent collections of file system paths to process. This enables you to do stuff like tell the compiler to compile files in two different folders at once, or to add multiple things to the classpath. This becomes useful because these locations can be anything you want (e.g. folders, or it could be a JAR or ZIP or WAR file, or anything else that you want to support). The FileManager API is designed to allow you to implement your own logic for how to query and index those collections of paths. An example for JCT is that we use a Google JIMFS in-memory file system for compiler sources and outputs, and we have some logic to read JAR files like a normal folder for classpaths.

Common paths include:

• StandardLocation.SOURCE_PATH - the paths to compile files within (e.g. src/main/java)
• StandardLocation.SOURCE_MODULE_PATH - the paths to compile nested modules in. This is rarely used because usually you just use one module in SOURCE_PATH like we do in this project's source code. The use case is basically having src/main/java/my.module1/... and src/main/java/my.module2/... to have multiple modules in the same project.
• StandardLocation.CLASS_PATH - The classpath.
• StandardLocation.MODULE_PATH - The module path (for JPMS modules).
• StandardLocation.SOURCE_OUTPUT - Generated sources get written here.
• StandardLocation.CLASS_OUTPUT - Generated classes and modules get written here.
• StandardLocation.ANNOTATION_PROCESSOR_PATH - Locations of compiled annotation processors.
• StandardLocation.ANNOTATION_PROCESSOR_MODULE_PATH - Locations of compiled annotation processors that are packaged in JPMS modules.

When you see locations, they are just simple handles that only contain name information. When those get provided to the file manager, the file manager can do things like find files matching specific criteria in specific locations (e.g. "find me all *.java files in SOURCE_PATH"), and reading and writing of files, classloading of stuff like annotation processors, etc.

Locations can be package-oriented (meaning each path is the root of a Java package file tree), or output-oriented (meaning they contain output files).

JPMS modules (that were introduced in Java 9) complicate this a bit further because the way the compiler has to treat these modules is that they live inside a "module-oriented" location (e.g. StandardLocation.SOURCE_MODULE_PATH), and these locations contain a bunch of "module locations" which themselves are package-oriented since they contain one package root each. This is what ModuleLocation is for, it basically represents the package-oriented location of a specific module, and all of these are kept in a module-oriented group. Interestingly, the compiler allows output-oriented locations to be both module-oriented and package-oriented.

Since we have three types of locations, "package-oriented", "module-oriented", and "output-oriented", the JCT file manager has to keep track of these locations separately. There are a few reasons for this:

1. The operations that are performed on package-oriented locations differs to module-oriented groups.
2. Some operations are expected to raise an error if you pass a module-oriented locations into them
3. The JCT implementation uses three different implementations of repositories (known as ContainerGroups in this project) that are represented by the locations.

The location objects themselves do not hold any file data, they are effectively just a wrapper around a name and a flag to say if they are package/module/output-oriented. Internally in the file manager, we use ContainerGroups to represent the contents of these locations. Each container group has a list of containers inside. Each container is representative of a path in that group.

So, example, if you compiled something and you had ~/.m2/org/example/Foo/1.0.0/Foo-1.0.0.jar and ~/.m2/org/example/Bar/1.0.0/Bar-1.0.0.jar on your classpath, the classpath would be represented like so:

JctFileManagerImpl(
  packages[CLASS_PATH] = PackageContainerGroupImpl([
    JarContainerImpl("~/.m2/org/example/Foo/1.0.0/Foo-1.0.0.jar"),
    JarContainerImpl("~/.m2/org/example/Bar/1.0.0/Bar-1.0.0.jar")
  ])
)

So, with all of this in mind (appreciate it is a tonne of technical info that seems overly complicated), those three location types have to be kept separate as part of how the API works. I don't think that at the moment there is much point in refactoring the file manager to represent these differently as it can introduce some weird behaviours if any details internally change, and I think we'd still have roughly the same amount of boilerplate regardless of what we do. The locations could all be stored in a single map I guess, but I kept them separate so that the logic that handles the three different types of location cannot get crossed over by mistake and lead to behaviour that Javac or ECJ wouldn't do themselves.

Hope that helps a little bit, if you have any questions just shout.

[ascopes]

Class diagram would look something along the lines of this:

(remember Location is just a handle to some group of paths, the ContainerGroup is the actual implementation of those groups; Containers wrap the individual paths to provide access to them in a consistent way)

[Badrri-Narayanan]

Wow, Thanks for the detailed explanation and UML diagram! This definetely provides me some context and I really really appreciate it. :)

[ascopes]

Hey, @Badrri-Narayanan, did you manage to get anywhere with this one? Since it is an important component, I might need to pick this one up at the weekend if not.

No pressure or anything, just means I know what I need to be working on :-)

Ideally want to release 0.0.1 by the start of March

[Badrri-Narayanan]

Hi @ascopes , I think this is a bit too complex for me. If you don't mind, can you please un-assign me this task? I am sorry I bit more than I could chew

[ascopes]

no problem, done!