Pass exceptions on to manufacture method #107

Nylle · 2024-08-22T20:44:17Z

Refs: #106

Also, chained assertions. Refs: #106

akutschera · 2024-08-25T12:47:55Z

src/test/java/com/github/nylle/javafixture/InstanceFactoryTest.java

@@ -120,6 +124,17 @@ void fallbackToFactoryMethodWhenConstructorThrowsException() {
            assertThat(result.getValue()).isNotNull();
        }

+        @Test
+        @DisplayName("will fallback to factory method and pass exceptions on")


This name is a bit misleading, because the class used in this test does not have a factory method.

If it had one and the factory method would succeed, then no exception would be thrown.
If it had one and the factory method would also throw an exception, the test is still green because the stack trace is the same (because we swallow the factory method exception here).

I'm not quite sure I understand.

The test-class does not have a factory-method intentionally - because it wouldn't throw otherwise. Yet we still fall back to a factory-method, because everything else failed.
So the intention is to have all approaches fail (the factory-method approach being the final one) and display the entire stack to show what we tried.

This test checks what happens if the constructor fails and the object has no factory method.

I would expect a different stack trace if a factory method was found but could not be successfully called because it also throws an exception.

This test checks what happens if the constructor fails and the object has no factory method.

Correct!

I would expect a different stack trace if a factory method was found but could not be successfully called because it also throws an exception.

Me, too. But this is not what the test is supposed to test. Are you saying there is a test missing?

Yes. We currently have no test for the unlikely case where a constructor method throws an exception and a factory method also throws one.
The more I think about it, the less I think this is ever going to be a problem.
We might just wait until someone files a bug report.

I added the test.

akutschera · 2024-08-25T12:48:20Z

This is the exception we would now get:

com.github.nylle.javafixture.SpecimenException: Cannot create instance of class <insert class name>: no factory-method found
	[... lots of class names not needed here...]
Caused by: com.github.nylle.javafixture.SpecimenException: No public constructor found
	... 79 more

To me, the Caused by makes it look like when we try to find a factory method, we failed because we could not find a public method.
What actually happened was that we first tried to find a public method and because we could not find one, we then tried to find a factory method.

What we are trying to tell the caller is that we found neither a public constructor nor a factory method (and the order in which we looked for them should not be important).

So while I really like the whole logic in this PR, I suggest we look for a better stack trace (maybe just one message?).

Nylle · 2024-08-25T12:56:44Z

So while I really like the whole logic in this PR, I suggest we look for a better stack trace (maybe just one message?).

Any suggestions? I'm aware that blowing up the stack trace is not perfect, but it seemed the easiest and quickest way to achieve the task.
But I'm open for improvements - hence this PR. What would you propose instead?

akutschera · 2024-08-25T13:32:21Z

src/main/java/com/github/nylle/javafixture/InstanceFactory.java

        return random.shuffled(type.getDeclaredConstructors())
                .stream()
                .filter(x -> Modifier.isPublic(x.getModifiers()))
                .findFirst()
                .map(x -> construct(type, x, customizationContext))
-                .orElseGet(() -> manufacture(type, customizationContext));
+                .orElseGet(() -> manufacture(type, customizationContext, new SpecimenException("No public constructor found")));


We should add the name of the class for which no public constructor was found. It could be that this is not the class I am fixturing but one of its fields.

Good point!

And if we add something like "trying factory methods next" to the message, it would improve the understandability of the stack trace.

Maybe we should not do exceptions at all and implement proper strategies?

I mean, this can certainly be improved. But do you think this is mergeable as a first attempt with improving it on further iterations?

Long comment short: Yes, I think we should try it.
Let's label it as experimental because it will only affect diagnosis, not behaviour.

akutschera · 2024-08-25T13:55:39Z

I am putting on my user hat:
When JavaFixture cannot fixture a class, I want to know what's wrong with the class, not what JavaFixture tried to do unsuccessfully.

With that in mind, the current PR gives me an answer to that. That is good.
What I currently get is a stack trace with e.g. three exceptions, two of which come from JavaFixture (constructing failed and factory method failed) and then the one from the actual class (constructor threw exception).

As a developer of JavaFixture I am happy that I have all three exceptions. It helps me understand where any future bugs might come from.

As user of JavaFixture that is more than I need, but maybe this is a good compromise.

Refs: #106

We may randomly create two Options.empty objects which are the same, so we cannot check if we cached any of these.

Nylle

fix: flaky test

Good catch, thank you!

feat: pass exceptions on to manufacture

97f6111

Refs: #106

Nylle requested a review from akutschera August 22, 2024 20:44

test: assert cause

736d4a2

Also, chained assertions. Refs: #106

akutschera reviewed Aug 25, 2024

View reviewed changes

Nylle and others added 2 commits August 25, 2024 16:50

test: increase test-coverage

f0cc7f2

Refs: #106

fix: flaky test

8f70cd9

We may randomly create two Options.empty objects which are the same, so we cannot check if we cached any of these.

Nylle commented Aug 25, 2024

View reviewed changes

akutschera merged commit 19af8b5 into master Aug 25, 2024
1 check passed

akutschera deleted the #106_Improve_error_descriptions branch August 25, 2024 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass exceptions on to manufacture method #107

Pass exceptions on to manufacture method #107

Nylle commented Aug 22, 2024

akutschera Aug 25, 2024

Nylle Aug 25, 2024

akutschera Aug 25, 2024

Nylle Aug 25, 2024

akutschera Aug 25, 2024

Nylle Aug 25, 2024

akutschera commented Aug 25, 2024 •

edited

Loading

Nylle commented Aug 25, 2024

akutschera Aug 25, 2024

Nylle Aug 25, 2024

akutschera Aug 25, 2024

Nylle Aug 25, 2024

akutschera Aug 25, 2024

akutschera commented Aug 25, 2024

Nylle left a comment •

edited

Loading

Pass exceptions on to manufacture method #107

Pass exceptions on to manufacture method #107

Conversation

Nylle commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutschera commented Aug 25, 2024 • edited Loading

Nylle commented Aug 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutschera commented Aug 25, 2024

Nylle left a comment • edited Loading

Choose a reason for hiding this comment

akutschera commented Aug 25, 2024 •

edited

Loading

Nylle left a comment •

edited

Loading