Making expected SQL annotations more descriptive and repeatable

[nagyesta]

Description
First of all, let me say this is a very useful library, great job!

I think it would be even better if the SQL annotations allowed us to add a comment to clarify why we are expecting to have that amount of queries. Plus it would be even better if they would be repeatable.

This would be great for those of us who are running a tight ship and want to clarify why we would expect exactly 42 selects to be executed in a test method. Currently the only option to do so is with comments, but those are not enforced and can become outdated.

If an annotation is repeated, their values should be summed up and then the sum should be compared with the actual detected select count.

Implementation ideas
I would like to do something like this:

    @Test
    @ExpectInsert(value = 3, comment = "Insert 3 users")
    @ExpectInsert(value = 10, comment = "Insert 10 comments")
    @ExpectInsert(value = 10, comment = "Insert 10 posts")
    @ExpectUpdate(value = 3, comment = "Update users")
    @ExpectSelect(value = 1, comment = "Load the user details")
    @ExpectSelect(value = 2, comment = "Load latest comments/posts")
    @ExpectSelect(value = 3, comment = "Load friend list and their latest comments/posts")
    @ExpectDelete(value = 3, comment = "Delete comments, users, posts")
    void test() {
        //...
    }

or alternatively

    @Test
    @ExpectInserts({
        @ExpectInsert(value = 3, comment = "Insert 3 users"),
        @ExpectInsert(value = 10, comment = "Insert 10 comments"),
        @ExpectInsert(value = 10, comment = "Insert 10 posts")
    })
    @ExpectUpdate(value = 3, comment = "Update users")
    @ExpectSelects({
        @ExpectSelect(value = 1, comment = "Load the user details"),
        @ExpectSelect(value = 2, comment = "Load latest comments/posts"),
        @ExpectSelect(value = 3, comment = "Load friend list and their latest comments/posts")
    })
    @ExpectDelete(value = 3, comment = "Delete comments, users, posts")
    void test() {
        //...
    }

If we provided 1 wrapper for each of the ExpectSelect, ExpectInsert, ExpectUpdate, ExpectDelete annotations, then it would be very flexible.

What do you think?

I would be happy to implement these changes if you like the idea...

[nagyesta]

Based on how the Annotations are collected, I think only the second option is viable (if it is possible at all).

[jeanbisutti]

What do you think about @ExpectInsert(value = 23, comment = "Insert 3 users, Insert 10 comments, Insert 10 posts"), with ideally the comment value re-used in the error message if 23 INSERT statements are not executed?

The ExpectJdbcQueryExecution annotation could also have a comment element (this annotation should probably be more promoted in the doc).

[nagyesta]

@ExpectInsert(value = 23, comment = "Insert 3 users, Insert 10 comments, Insert 10 posts")

I think this is not really different from the current situation where we need to add the comment next to the annotation. The main problem is the passing of time and people forgetting to update the comment. In the end it will look like this over time:

@ExpectInsert(value = 31, comment = "Insert 3 users, Insert 10 comments, Insert 10 posts")

With the proposed repeated annotation this goes away because we never calculate the sum, we are just specifying what kinds of queries we have and the machine will do the hard part for us (as it should).

[nagyesta]

Also, happy to add a comment to ExpectJdbcQueryExecution too.

[jeanbisutti]

The main problem is the passing of time and people forgetting to update the comment.

Your proposal seems to have the same issue.

With

    @ExpectSelects({
        @ExpectSelect(value = 1, comment = "Load the user details"),
        @ExpectSelect(value = 2, comment = "Load latest comments/posts"),
        @ExpectSelect(value = 3, comment = "Load friend list and their latest comments/posts")
    })

it's more difficult to very quickly figure out that 6 SELECT statements are executed.

@ExpectSelect(value = 2, comment = "Load latest comments/posts"),

To be sure that these 3 SELECT statements are related to "Load latest comments/posts" it would be needed to be able to inspect the content of the SELECT statements.

[nagyesta]

Hi @jeanbisutti ,
I think we are not talking about the same use-case and same mechanism so please allow me to clarify.

If I have a complex test case like this (please keep in mind that the comments starting with NOTE: are only added so that you can understand the complexity of the tested code):

void testLoadingActivityPageContentShouldBeSuccessfulWhenWeHaveMoreUsersPostAndComments() {
    //given
   User user1 = createRandomUser(); //NOTE: saves a user
   User user2 = createRandomUser();
   User user3 = createRandomUser();
   Post user1post1 = createRandomPostForAuthors(user1); //NOTE: saves a post and adds relations in the user_post join table
   Post user1post2 = createRandomPostForAuthors(user1, user3);
   Post user2post1 = createRandomPostForAuthors(user2);
   Post irrelevantPost = createRandomPostForAuthors(user3);
   Comment comment1 = createRandomCommentOnPostForUser(user1post1, user1); //NOTE: saves a comment (uses join columns)
   Comment comment1Reply = createRandomCommentReplyOnCommentForUser(comment1, user2); //NOTE: saves a comment (uses join columns)
   Comment comment2 = createRandomCommentOnPostForUser(user1post2, user3);
   Comment comment2Reply = createRandomCommentReplyOnCommentForUser(comment2, user1);
   Comment comment3 = createRandomCommentOnPostForUser(user2post1, user1);
   Comment irrelevantComment = createRandomCommentOnPostForUser(irrelevantPost, user2);
   Comment irrelevantCommentReply = createRandomCommentReplyOnCommentForUser(irrelevantComment, user3);

   //when
   //NOTE: load the user details of user1
   //NOTE: load all posts where user1 is author
   //NOTE: load all comments where user1 is the author
   //NOTE: load all comments this are in the same threads as the ones loaded previously
   //NOTE: load user info of all comment authors
   //NOTE: load the links of all posts on which the previously loaded comment threads are made
   ActivityPageModel actual = loadActivityPageModelAsUser(user1);

   //then
   //...assertions
   deleteAllUsers(); //NOTE: only deletes users
   deleteAllPosts(); //NOTE: deletes posts and comments using cascade
}

Today, with the latest QuickPerf, when I start adding the annotations I can only go through the whole source like I did when I added the NOTE: annotations in this case, then sum up the number of inserts, selects, updates and add the following annotations:

//insert 3 users + 4 posts + 5 relationships + 7 comments = 19
@ExpectInsert(19)
//select 1 user details + 2 posts (in 1 query) + 3 comments (in 1 query) + the rest of the threads (1 query) + user infos (1 query) + post links (1 query)
@ExpectSelect(6)
//delete all users (1) + delete all posts and comments (1 query using cascade)
@ExpectDelete(2)

When the logic changes I have to hope that the guy before me updated the comment accurately otherwise I need to either recalculate the whole thing in my head or get a calculator and sum up the queries because nothing is enforcing any level of accuracy of the comments.

With my proposal, I could simply add the following annotations:

@ExpectInserts({
    @ExpectInsert(comment="Create 3 users", value="3"),
    @ExpectInsert(comment="Create 4 posts and 5 author relations", value="9"),
    @ExpectInsert(comment="Create 7 comments", value="7")
})
@ExpectSelects({
    @ExpectSelect(comment="Load user details"),
    @ExpectSelect(comment="Load posts where the user is author"),
    @ExpectSelect(comment="Load comments left by the user"),
    @ExpectSelect(comment="Load related comment threads"),
    @ExpectSelect(comment="Load user info for comment owners"),
    @ExpectSelect(comment="Load post links for comments")
})
@ExpectDeletes({
    @ExpectDelete(comment="Delete all users (no cascade)"),
    @ExpectDelete(comment="Delete all posts and comments using cascade")
})

In my opinion it is very simple and natural this way to only look at the lines of the code and write what you see without the need to calculate the totals.

Then, when there is a test failure I will get the usual message, saying that I would expect that I had 19 inserts but it was 21. This tells me that I am off by 2 so I need to find a part of the code where that might have happened. If we are talking about new code, I can compare my annotations and queries to find it, otherwise if a regression caused the failure, then I should already know which part I have changed and simply check that relevant annotation.

This is what I meant by my previous remarks and why I still think, that there is no need to do any further inspections on the select contents, we can just let the computer do the calculation that sums up the total number of expected queries (as my related PR #225 does) and compare it with the number of observed queries (same as it happens today when using the already existing annotations).

It feels a bit weird to ask whether I am missing something (in my own use case with which I am working daily) but I cannot rule out that I am simply using QuickPerf the wrong way for example, so I am open to your suggestions. Is there a way to:

1. Avoid doing the manual calculation to sum up the total number of queries
2. Ensure that the comments and the numbers will remain in sync over time
3. Keep the same level of accuracy/detail during the QuickPerf verification steps?

[jeanbisutti]

Based on

@ExpectInserts({
   @ExpectInsert(comment="Create 3 users", value="3"),
   @ExpectInsert(comment="Create 4 posts and 5 author relations", value="9"),
   @ExpectInsert(comment="Create 7 comments", value="7")
})

,

if a regression leads to the insertion of 2 users and 8 comments, the test is always green and the comment values can be misleading.

@ExpectInsert(comment = "We expect the creation of 3 users, 4 posts, 5 author relations, 7 comments", value = "20") may be less misleading because the comment value is related to the users, posts, author relations and comments.

[nagyesta]

Sorry @jeanbisutti , I am not sure I can see your concern based on your example!

I agree, that in case somehow we had only 2 user inserts and 8 comments instead of the expected 3 users and 7 comments, then QuickPerf wouldn't notice it and would not prevent the test from passing. At the same time, the exact same thing happens if I have to manually sum up the total and only use one annotation as shown in your alternative, so I am not really sure what your point was.

Nonetheless, I am a bit puzzled, because I think if I wrote so bad tests that none of my assertions found out if I created less users and more comments than intended, then I think I wouldn't be concerned about the performance implications either and I wouldn't even have QuickPerf on my classpath. Either way, this would be a clear issue with the functionality and I was under the impression, that the purpose of QuickPerf is to evaluate some performance-related properties instead of finding functionality issues. Is that not correct?

Also, I think we might have a different idea about the meaning of these comments. Somehow it feels like you would like to handle them as failure messages. Is that the intention? If yes, maybe we should name it as message, instead of comment (following in the footsteps of many test frameworks using the same terminology in their assertion methods).
My intention was to add some additional meaning to the value for the developer who is looking at the code AND make it almost impossible to miss it or detach it from the number in the same annotation. That makes it harder to miss it when the number is updated.

If the comment is never used in the error message used by QuickPerf, because we keep using the familiar wording:

[PERF] You may think that <19> insert statement was sent to the database
But there are in fact <20>...

then I would argue, that there is nothing misleading, because we think that we have a total of 19 inserts, but there were 20.

When we see the error and we go to the test method, then we can even see from the comments why we thought that this will be the case and sooner or later we will see why we were wrong. The whole point is that the comments must be kept up-to-date to help us, because as soon as I cannot trust the comments because someone forgot to update them or made a calculation error (like you did in your example when you have summed up 3+4+5+7=20 instead of 19) then it is IMHO better if I have no comments at all.

To offer a way forward, I can see the following options which would solve the problem I am facing:

1. If I was so convincing above that you decide to merge 😄
2. I can add an optional matching attribute to each of the ExpectSelect/ExpectInsert/ExpectUpdate/ExpectDelete annotations, letting the user define some regexp or similar pattern which we can match against the queries, count only the matching ones and compare the result with the value from the annotation. This would allow even deeper analysis and we could clearly see (and report) which queries are not expected. Also, if the total number of inserts (regardless of matching) is not equal to the sum of all ExpectInsert values, then we can report that too as an error.
3. I can rename the comment attribute to message and find a way to compose a message using them when the evaluation fails, but still allow repeating the annotations as proposed.

What do you think? Could any of these work for you?

[jeanbisutti]

What you propose seems more a comment/message than an expectation/assertion, see #222 (comment). Using an Insert annotation into an Insert annotation to do this seems odd to me.

In addition, the sum concept would make the annotation a bit more difficult to read and understand. @ExpectInsert(value = 23, comment = "Insert 3 users, Insert 10 comments, Insert 10 posts") seems shorter and easier to read for me.