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.

Sign up for GitHub

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

Jump to bottom

convert instrument jdbc test from groovy to java #12132

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

convert instrument jdbc test from groovy to java #12132

wants to merge 9 commits into from

Conversation

shalk
Copy link
Contributor

@shalk shalk commented Aug 28, 2024 •
edited
Loading

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
jaydeluca
jaydeluca reviewed Sep 9, 2024
View reviewed changes
...n/jdbc/testing/src/main/java/io/opentelemetry/instrumentation/jdbc/TestDatabaseMetaData.java Outdated Show resolved Hide resolved
...ntation/jdbc/testing/src/main/java/io/opentelemetry/instrumentation/jdbc/TestConnection.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
shalk and others added 2 commits September 10, 2024 14:13
@shalk
fix ut
f02b3e6
@shalk @jaydeluca
Apply suggestions from code review
5c7af8c 
Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>
jaydeluca
jaydeluca reviewed Sep 13, 2024
View reviewed changes
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Show resolved Hide resolved
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java Outdated Show resolved Hide resolved
shalk and others added 2 commits September 13, 2024 19:22
@shalk @jaydeluca
Apply suggestions from code review
11eea16 
Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>
@shalk
up level nested class
635484d
@shalk
Copy link
Contributor Author

shalk commented Sep 14, 2024 •
edited
Loading

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

laurit
laurit reviewed Sep 17, 2024
View reviewed changes
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java
Comment on lines +81 to +98
dbName = "jdbcUnitTest";
dbNameLower = dbName.toLowerCase(Locale.ROOT);
jdbcUrls =
ImmutableMap.of(
"h2", "jdbc:h2:mem:" + dbName,
"derby", "jdbc:derby:memory:" + dbName,
"hsqldb", "jdbc:hsqldb:mem:" + dbName);

jdbcDriverClassNames =
ImmutableMap.of(
"h2", "org.h2.Driver",
"derby", "org.apache.derby.jdbc.EmbeddedDriver",
"hsqldb", "org.hsqldb.jdbc.JDBCDriver");

jdbcUserNames = new HashMap<>();
jdbcUserNames.put("derby", "APP");
jdbcUserNames.put("h2", null);
jdbcUserNames.put("hsqldb", "SA");
Copy link
Contributor

Choose a reason for hiding this comment

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

you could have initialized these in the field declaration as the groovy test did

laurit
laurit reviewed Sep 17, 2024
View reviewed changes
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java
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

laurit
laurit reviewed Sep 17, 2024
View reviewed changes
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java
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.

laurit
laurit reviewed Sep 17, 2024
View reviewed changes
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java
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);

laurit
laurit reviewed Sep 17, 2024
View reviewed changes
.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java
Comment on lines +1069 to +1072
@SuppressWarnings("deprecation") // TODO DbIncubatingAttributes.DB_CONNECTION_STRING deprecation
AttributeKey<String> getDbConnectionStringKey() {
return DbIncubatingAttributes.DB_CONNECTION_STRING;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps instead of a method it would be better to define this as a field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurit laurit laurit left review comments

@jaydeluca jaydeluca jaydeluca approved these changes

Assignees
No one assigned
Labels
area:tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shalk @laurit @jaydeluca @SylvainJuge