Issue 46

[Marc Gomez]

Hi Graham,

So finally I found some time to get deeper into the code and start working on issue 46 (https://github.com/MutabilityDetector/MutabilityDetector/issues/46).  I've written a small test for 3 checkers, which basically asserts the locations emitted for each possible error. After fighting a little bit with git I was able to send you a pull request so that you can take a look. Please let me know if I am going in the right direction.

Cheers,

 Marc

[Graham Allan]

Hi Marc,

Good job on fighting git, and winning :-)

The pull request is not quite what I was looking for. The tests you wrote, while high quality (good job on finding test utils, etc) the coverage for the checkers creating the CodeLocations already exist[0][1][2]. 

What I was intending with this issue was to write tests for classes, as if you were a user of MD, then report back on whether you think the code location used is the most appropriate, and if it can be improved. I was expecting more of a 'report' than a pull request. Using your viewpoint as a new user, you can provide fresh perspective on whether or not the existing code locations produced are as helpful as they could be. E.g:

 - ArrayFieldMutabilityChecker: code location points to the field, which is the best place, but only identifies it by name, and doesn't provide a line number.

 - etc

With a comprehensive list of all the code locations produced, we can decide together which code locations need to be improved, and where we're going to retrieve line numbers from, etc, and we will have a better idea on how much work is required for #43. 

The tests you have written are for internal classes, which I _believe_ are all already tested. If you do find missing coverage for any of the checkers, adding tests for those would be great, but 

Well done on getting feedback early, very agile :-)

Is that okay?

Regards,

Graham A

[0] ArrayFieldMutabilityCheckerTest https://github.com/MutabilityDetector/MutabilityDetector/blob/master/src/test/java/org/mutabilitydetector/benchmarks/mutabletofield/array/ArrayFieldMutabilityCheckerTest.java#L70

[1] CanSubclassCheckerTest https://github.com/MutabilityDetector/MutabilityDetector/blob/master/src/test/java/org/mutabilitydetector/benchmarks/CanSubclassCheckerTest.java#L89

[2] https://github.com/MutabilityDetector/MutabilityDetector/blob/master/src/test/java/org/mutabilitydetector/benchmarks/InherentTypeMutabilityCheckerTest.java#L88

--
You received this message because you are subscribed to the Google Groups "mutability-detector" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mutability-dete...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Marc Gomez]

Hi Graham,

Oops, I didn't realize you had already implemented these tests. I actually started writing the tests from an MD user perspective, but then I decided to write specific tests for each checker, so that I can audit code locations focusing on only on one checker at a time (for some cases I found it hard to get only 1 mutability reason otherwise). Anyways now I will concentrate more in the audit than in the test, I hope I can find some time this week for this.

.. and many thanks for your motivating feedback ;-)

Cheers,

 Marc

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-detector+unsub...@googlegroups.com.

[Graham Allan]

Hi Marc,

Were you able to make any progress? If you haven't been able to find the time, that is absolutely fine, I just want to make sure you're not blocked on anything.

~ Graham A

[Marc Gomez]

Hi Graham,

I indeed found some time last Sunday, I has not able to finish though and I wanted to continue tomorrow :-). Here the current status:

https://github.com/marcgs/ClientOfMutabilityDetector/blob/master/src/test/java/org/mutabilitydetector/ErrorLocationTest.java

As you see I wrote a test from the user perspective for some of the checkers. In the test itself I started commenting what could be improved in the error locations. 

In general I found the current code locations correct, perhaps line numbers would be helpful in some cases as you mentioned. Other minor improvements are as well listed in the test. Let me know your opinion!

Cheers,

 Marc

You received this message because you are subscribed to a topic in the Google Groups "mutability-detector" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/mutability-detector/nTRsf3us4O0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to mutability-dete...@googlegroups.com.

[Graham Allan]

Hi Marc,

That's much closer to what I was expecting, brilliant! It seems like there's two types of improvement that can be made: adding line numbers (as expected); but also, condensing several errors into one. The latter is something I haven't thought much about, but it could be a similar (separate) enhancement.

Good work :)

~ Graham A

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-dete...@googlegroups.com.

[Marc Gomez]

Hi Graham,

So here is a first version of the audit. I've covered all checkers excepting the SetterMethodChecker since I am not sure if it is meant to be used or not. Please refer to the following test for examples:

https://github.com/marcgs/ClientOfMutabilityDetector/blob/master/src/test/java/org/mutabilitydetector/ErrorLocationTest.java

Findings:

- Line numbers whenever possible

- Consolidate multiple reasons. As I understand the reason for this design is to be able to allow exceptions granularly through AllowedReasons. If no AllowedReasons are used the output can be quite verbose though. Examples are:

  1) ABSTRACT_TYPE_INHERENTLY_MUTABLE + CAN_BE_SUBCLASSED for abstract classes

  2) PUBLISHED_NON_FINAL_FIELD + NON_FINAL_FIELD for published non final fields

  3) FIELD_CAN_BE_REASSIGNED + NON_FINAL_FIELD for fields which are modified in a method

- MutableTypeToFieldChecker->ClassNotWrappingCollection: if a collection is not being wrapped at all the error message still mentions "attemps to wrap mutable collection..." which IMHO is misleading.

- MutableTypeToFieldChecker->CollectionWrappingWithoutCopy: It would be helpful to have the assignment displayed where the copy should be done (and perhaps an short hint how)

- MutableTypeToFieldChecker->CyclicDependency2Classes: I was not able to get a cyclic dependency error for 2 classes

Let me know what you think.

Cheers,

 Marc

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-detector+unsubscribe...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

--

You received this message because you are subscribed to a topic in the Google Groups "mutability-detector" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/mutability-detector/nTRsf3us4O0/unsubscribe.

To unsubscribe from this group and all its topics, send an email to mutability-detector+unsub...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Graham Allan]

Fantastic.

Could you raise a GitHub for each of: a) consolidating the reasons; b) misleading 'attempts to wrap mutable collection' message; and c) improving the error message to include a hint on how to copy.

Finding a cyclic dependency is tricky, I'm not exactly sure what triggers it. Constructor and Class are a pair that I've seen it happen with, but mainly the check+error message is to guard against StackOverflowErrors :)

If you're happy to, could you leave issues b and c (they will be used for newer newcomers), and work on a? 

Cheers,

Graham 

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-dete...@googlegroups.com.

[Marc Gomez]

Hi Graham,

Cool! I've created the following issues:

https://github.com/MutabilityDetector/MutabilityDetector/issues/51

https://github.com/MutabilityDetector/MutabilityDetector/issues/52

https://github.com/MutabilityDetector/MutabilityDetector/issues/53

I will start working on 53, I'll get back to you as soon as I have a better idea how to proceed there. Btw... do you want me to issue a pull request for the ErrorLocationTest I wrote for the audit?

Cheers,

 Marc 

To unsubscribe from this group and all its topics, send an email to mutability-detector+unsubscribe...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

--
You received this message because you are subscribed to the Google Groups "mutability-detector" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mutability-detector+unsubscribe...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

--

[Graham Allan]

Great stuff, and yes please for the pull request.

To unsubscribe from this group and stop receiving emails from it, send an email to mutability-dete...@googlegroups.com.