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

Unit tests for operation attempt #1472

Merged
merged 29 commits into from
Oct 3, 2024
Merged

Conversation

amorton
Copy link
Contributor

@amorton amorton commented Sep 30, 2024

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

for #1424, will need tablemetadta on the collectionschema object

it was a giant mess, still is, but trying to cleanup gradually
runs, not all unit tests checked

Adds OperationAttempt as a base "task" that any operation runs,
example in InsertTableOperation and InsertManyCommandResolver

No tests written yet.
Because there are several ways we create this, the builder is used in
the work for OperationAttempt
added driver profile
reset logging to Debug
tweak some error handle and retry because it had issues
@amorton amorton requested a review from a team as a code owner September 30, 2024 04:16
@amorton amorton changed the title Unit tests for operation attempt WIP - Unit tests for operation attempt Sep 30, 2024
import java.util.HashMap;
import java.util.Map;

public class TestData {
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax Oct 1, 2024

Choose a reason for hiding this comment

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

This could use Javadoc explaining its raison d'etre.

But overall it seems like a complicated abstraction for doing... something.
It seems to me that eager direct construction of XxxTestData classes would work fine and I wonder if this is a solution looking for problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it's still a bit WIP, the idea was to share things like the names so they can be re-used

try {
Constructor<?> constructor = clazz.getConstructor(TestData.class);
return constructor.newInstance(this);
} catch (NoSuchMethodException
Copy link
Contributor

Choose a reason for hiding this comment

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

just catch (Exception e) and be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update shortly in another PR to use the wider catch

}

@SuppressWarnings("unchecked")
private <T> T getOrCache(Class<T> clazz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really make sense over eagerly constructed instances? It's not tons of code but... is there a real performance (or some other) problem being solved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea was to share the same instances of things, such as the names

Copy link
Contributor

@tatu-at-datastax tatu-at-datastax left a comment

Choose a reason for hiding this comment

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

Overall fine, but I find TestData abstraction bit odd -- it's not tons of code, but it also seems to be solving problem I don't quite see.

@amorton amorton changed the title WIP - Unit tests for operation attempt Unit tests for operation attempt Oct 3, 2024
@amorton amorton merged commit b8f9a98 into main Oct 3, 2024
3 checks passed
@amorton amorton deleted the ajm/unit-tests-for-OperationAttempt branch October 3, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants