Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

convert instrument jdbc test from groovy to java #12132

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

shalk
Copy link
Contributor

@shalk shalk commented Aug 28, 2024

relate to #7195

@shalk shalk marked this pull request as ready for review September 6, 2024 10:07
@shalk shalk requested a review from a team September 6, 2024 10:07
shalk and others added 2 commits September 10, 2024 14:13
Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>
shalk and others added 2 commits September 13, 2024 19:22
Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>
@shalk
Copy link
Contributor Author

shalk commented Sep 14, 2024

It is look like

:instrumentation:couchbase:couchbase-2.6:javaagent:test

failed by Address already in use.

It is not break by jdbc test. the jdc case is all memory server

Comment on lines +342 to +343
@DisplayName(
"basic statement with #connection.getClass().getCanonicalName() on #system generates spans")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the value of this. Spock is able to replace #connection.getClass().getCanonicalName() and #system with actual parameter values so you'd have a test named prepared statement execute on h2 with org.h2.jdbc.JdbcConnection generates a span with junit this does not happen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

junit can not do the same ability,like getCannicalname。

Junit is a tree struct, DisplayName for the method, parametertest have each name。

I do not found the usage in other tests in the projects。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you understood. When you run the groovy tests in intellij then the main test is named basic statement with #connection.getClass().getCanonicalName() on #system generates spans when you expand it then you see the test with parameters. They are named basic statement with org.h2.jdbc.JdbcConnection on h2 generates spans. When you run the junit test the main test name is what you set in display name and when you expand it you'll just see the parameters. That is why I believe that using the @DisplayName they way you do does not make sense. There is no magic interpolation like in spock. You might either not add @DisplayName at all or change it to @DisplayName("basic statement generates spans")

Copy link
Contributor Author

@shalk shalk Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will remove @DisplayName

Comment on lines +373 to +378
satisfies(
DbIncubatingAttributes.DB_USER,
val ->
val.satisfiesAnyOf(
v -> assertThat(v).isEqualTo(username),
v -> assertThat(v).isNull())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original groovy test uses

if (username != null) {
  "$DbIncubatingAttributes.DB_USER" username
}

If username is given verify that this attribute exists, your assertion just makes user name optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I can tell you only fixed it in some places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me see

Comment on lines 383 to 384
statement.close();
connection.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other tests use

    cleanup.deferCleanup(statement);
    cleanup.deferCleanup(connection);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

junit will autoclose connection on method parameter

I only add

 cleanup.deferCleanup(statement);

Copy link
Contributor

@laurit laurit Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is fine.
To me it feels a bit odd that there is a bunch of tests where connection is passes as parameter and still use cleanup.deferCleanup(connection); and there is one test that does not do it. Perhaps remove it from other tests also where it is not needed?

@shalk shalk requested a review from a team as a code owner September 23, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants