search

MetroCS / imagelab

ImageLab
GNU General Public License v3.0
7 stars 80 forks source link

[UserStory] Update build file to run and report results of JUnit tests #44

Closed robo-jones closed 4 years ago

robo-jones commented 4 years ago

User Story

Essential components

Story

As a maintainer I want gradle to automatically build and run unit tests with JUnit4 so that the project's code quality can be improved through automated unit testing.

Acceptance Criteria

Supporting Information

Currently, there is incomplete JUnit4 support in the project's build.gradle file that indicates an intention to have support for automated JUnit4 testing in the project. This user story is intended to "finish" that support.

Dependencies

Dependents

4 [UserStory] Add unit tests for existing classes

jody commented 4 years ago

I'm a bit confused. Is this about adding unit tests for existing classes or about project compatibility with JUnit4?

robo-jones commented 4 years ago

This issue is about compatibility with JUnit4, and specifically about integrating it into the automated build system (gradle).

As the project currently stands, gradle will not do anything with unit tests except compile them; it will not run them, or generate any sort of useful report or build messages.

The intent is that, once this issue is corrected, there will be a sort of "scaffold" in place that will allow other contributors/maintainers who create unit tests to have those tests be ran automatically via gradle, and to have a useful report be generated so that those tests can be easily used to drive code quality.

EDIT: The necessary plugins to do all of this are already incorporated, they just haven't been properly "wired up".

jody commented 4 years ago

Thanks for the clarification! I've changed the title of this issue to, "Update build file to run and report results of JUnit tests". Since the original issue specifies JUnit4, that is sufficient for an effective solution.

An enhanced solution would be one that handles both JUnit4 and JUnit5 tests.

It is more important to get JUnit4 tests working soonest.

The upgrade to JUnit5 framework can be handled afterwards as a separate issue. In the future, the project would want to convert it's existing JUnit4 tests to JUnit5 as well.

robo-jones commented 4 years ago

I believe I have a working solution to implement this user story, however there is a bit of an issue: gradle is reasonably smart, and so if it does not find any unit tests, it will not generate any unit test report. Currently, the project has no unit tests. Thus, in order to verify that this user story has been properly implemented, some sort of unit test is required.

To keep this story in scope, I was thinking of adding a "dummy" unit test that always passed (assertEquals(1, 1); or something of that nature). However, since that is neither specified in the original requirements of this user story, nor a particularly good practice, I wanted to ask for your feedback on this matter. I could simply commit the build.gradle changes and let future maintainers take it on faith that they work, I could add the dummy unit test (along with an issue to get rid of it whenever somebody commits the first actual unit tests), or we could come up with something else.

jody commented 4 years ago

I think the shortcoming is that this issue needs a way to verify a proposed solution. This issue couldn't be closed until the proffered solution was verified.

I agree that it is inappropriate to add arbitrary meaningless tests, that have no place in the code base and would need to be removed.

But how difficult is it to add simple meaningful tests? They need not be sufficient to address existing issues (#6, #8, #16, or epic #4), just something that belongs in the code base.

That seems like a simple task to make a component dependency of this issue.

robo-jones commented 4 years ago

That sounds good to me! I have modified the acceptance criteria to reflect it.

jody commented 4 years ago

Note that the ImageLab project includes package.bluej files and the structure is consistent with BlueJ's expectations.

This issue may need to ensure that any solutions pose no conflict with other stakeholders' needs. For example, does the testing structure need to be consistent with use of BlueJ as well as Gradle?

robo-jones commented 4 years ago

So, after researching further, it is a simple matter to store unit test files in the same directory as their associated classes, as required by BlueJ. However, there is still a non-trivial obstacle to maintaining BlueJ support with this user story: BlueJ will not recognize that a unit test class is associated with its parent class unless the appropriate package.bluej file is updated to reflect this association. This means that any future unit test classes that are not created using BlueJ will require the appropriate package.bluej file to be updated separately.

There are 4 possible resolutions that I can think of, all of which have some wrinkles:

  1. Add a function to the build system to automatically handle the package.bluej updates.
    • Has the potential to be a rather complex and involved task, and should probably be its own User Story.
    • However, as long as the overall structure is properly maintained in the meantime, this can be done later.
  2. Require contributors to take care of updating package.bluej (either by using BlueJ to create all future classes, or making the appropriate changes manually).
    • Requires changes to the project's documentation to reflect this requirement.
    • Requires reviewers to ensure that the requirement has been met on all future unit test related PRs.
  3. Accept that there will be imperfect BlueJ support.
    • As long as the unit tests are integrated into the build system, they can still be utilized by BlueJ users.
  4. Drop BlueJ support from the project.
    • This is a philosophical issue: should the project be so closely tied to a specific IDE?

I can create skeleton unit test classes for all the current project classes, which ought to allow this decision to be delayed for a while, but in the mean time, if anyone decides to contribute a new class (such as a new filter) along with unit tests for that class, there is a potential for BlueJ support to be broken by their contribution.

robo-jones commented 4 years ago

Just posting a comment to bring this to your attention, as you requested.

jody commented 4 years ago

... store unit test files in the same directory as their associated class files ...

  1. Accept that there will be imperfect BlueJ support.
    • As long as the unit tests are integrated into the build system, they can still be utilized by BlueJ users.

This solution approach and directory organization seem most palatable to me. It is OK for the test classes to not have "tight coupling" with the implementation classes (e.g., via package.bluej), as long as the test classes get compiled and run properly using the standard user interfaces. This directory organization has the added advantage of a more obvious association of implementation and test classes regardless of the build and development tool.

I can create skeleton unit test classes for all the current project classes

I think it is better to create only contentful unit test classes. That is, add a test class only when it contains a meaningful test.

Template(s) for unit test classes for the ImageLab project could be valuable and should be documented for application to any implementation class, now and in the future.

robo-jones commented 4 years ago

I've updated the issue to reflect these requirements.

However,

Template(s) for unit test classes for the ImageLab project could be valuable and should be documented for application to any implementation class, now and in the future.

I agree that this would be quite valuable; do you think it should be rolled into this issue? If not, removed, as per our in-person discussion today

I believe I can address this issue as it stands within the next couple of days, and would like to do so.