adds a test case for testing module location in file manager

[Badrri-Narayanan]

Description

Adds a test case for adding module location in JCT file manager.

Issues

contribution towards #270

[codecov-commenter]

Codecov Report

Merging #272 (3d45d06) into main (1cb4e14) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #272   +/-   ##
=======================================
  Coverage   71.17%   71.17%           
=======================================
  Files          74       74           
  Lines        2560     2560           
  Branches      238      238           
=======================================
  Hits         1822     1822           
  Misses        671      671           
  Partials       67       67           

[ascopes]

Is this the same as just var moduleLocation = StandardLocation.MODULE_PATH?

[Badrri-Narayanan]

Thanks. I will make the change

[ascopes]

should be able to just say .isTrue rather than .isEqualTo(true) here to keep it a little more concise

[Badrri-Narayanan]

Will do

[ascopes]

Since this flow hits this line of code: https://github.com/ascopes/java-compiler-testing/blob/main/java-compiler-testing/src/main/java/io/github/ascopes/jct/filemanagers/impl/JctFileManagerImpl.java#L97, do we want to test that the modules get detected correctly too?

Effectively, if I give this method a module oriented location and a path, I want to test that it finds the modules in that path and adds them as separate paths to the module oriented container group.

Ideally, since this is a unit test for the file manager, I should rely on the container group classes working as little as possible. If you check other test classes, I use mockito to basically stub the behaviour I expect in various situations on the objects I interact with, and then inject them in the places I want. Testing how these components all link together and work together properly would be more suited to the integration tests package instead.

This might be a bit more complicated to test, since ModuleFinder is part of the JRE and looks for nested directories that contain a module-info.class inside them.

What you could do to avoid having to make a bunch of files and classes in the test case is make a static mock of ModuleFinder... this is a bit involved but bare with me.

So we want to:

• mock ModuleFinder.of() to return a fake stub that we control so we can manipulate how it behaves and verify certain method calls are made.
• mock the fake stub module finder so .findAll returns some set of module references we control.
• make a fixture so we can make ModuleReference objects easily.

The fixture is going to be the easiest one. There is a static class called Fixtures in the helpers package in the tests. We want to make a method to take a Path that we provide and return some ModuleReference that we mock.

Something like this should work:

static ModuleReference someModuleReference(Path path) {
  var ref = mock(ModuleReference.class);
  when(ref.location()).thenReturn(path.toUri());
  return ref;
}

The mocking of ModuleFinder will be a bit more complex.

// Replace ModuleFinder's static methods
// with some that we programmatically stub.
try (var moduleFinderMock = mockStatic(ModuleFinder.class)) {

  // This will be what we return from
  // ModuleFinder.of(...);
  var moduleFinder = mock(ModuleFinder.class);

  // Make a stub.
  // When I call ModuleFinder.of(...)
  // Then return our ModuleFinder mock.
  moduleFinderMock
      .when(() -> ModuleFinder.of(any()))
      .thenReturn(moduleFinder);

  // Stub the result of .findAll()
  // When I call .findAll()
  // Then return a set of module references
  // to some module paths we make.
  var modulePath1 = somePath();
  var modulePath2 = somePath();
  var modulePath3 = somePath();

  when(moduleFinderMock.findAll())
      .thenReturn(Set.of(
          someModuleReference(modulePath1),
          someModuleReference(modulePath2),
          someModuleReference(modulePath3)
      ));

  // Your logic in this PR goes here <--

What we can then do is assert that the expected paths are present in the result by getting the module container group for the location in the file manager, and then querying the result.

It might be simpler to also consider mocking the construction of the ModuleContainerGroupImpl class and just verify that .addModule() is called with the arguments you expect via Mockito.

This is probably going to be the most complex class in the API to mock since there is a tonne of integration with lots of other moving parts here. The other place that does a lot of this which may be of interest to see how I dealt with testing that is at https://github.com/ascopes/java-compiler-testing/blob/main/java-compiler-testing/src/main/java/io/github/ascopes/jct/compilers/impl/JctJsr199Interop.java, the corresponding tests are at https://github.com/ascopes/java-compiler-testing/blob/main/java-compiler-testing/src/test/java/io/github/ascopes/jct/tests/unit/compilers/impl/JctJsr199InteropTest.java#L148

[Badrri-Narayanan]

@ascopes Thank you so much for your valuable inputs! :)

[ascopes]

Just a note that #279 changes the structure of some stuff that interacts with code in this PR to try and simplify some of these interactions a bit from a testing perspective. If you see any conflicts as a result of that, then that is my fault and just shout if you need help resolving anything.

[ascopes]

Okay so, about to push a new change. That abstracts away part of the complexity of the class you are writing tests for.

As an example, the tests you wrote previously now become a lot simpler with this change. For example:

  @Test
  @DisplayName(".addPath(Location, PathRoot) adds the path to the repository")
  void addPathAddsThePathToTheRepository() {
    // Given
    var location = someLocation();
    var pathRoot = mock(PathRoot.class);

    // When
    fileManager.addPath(location, pathRoot);

    // Then
    verify(repository).addPath(location, pathRoot);
  }

I'll put a PR up for that and merge it, probably worth rebasing onto that and seeing if it is any clearer now than the huge explaination I had to give before due to the complexity of it internally. Should be a lot simpler now :-)

[ascopes]

As mentioned from last week; API has been simplified, so this will need tweaking to match that. Writing the tests should be far less complex now though.

[ascopes]

closed per #270 comments