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

Migrating datastore tests to JUnit5 #1031

Merged
merged 3 commits into from
Mar 24, 2022
Merged

Migrating datastore tests to JUnit5 #1031

merged 3 commits into from
Mar 24, 2022

Conversation

ddixit14
Copy link
Contributor

Tests Migrated:

  1. DatastoreIntegrationTests.java
  2. ParallelDatastoreIntegrationTests.java
  3. SubclassesDescendantsIntegrationTests.java
  4. SubclassesReferencesIntegrationTests.java
  5. PartTreeDatastoreQueryTests.java
  6. DatastoreRepositoryFactoryTests.java
  7. SimpleDatastoreRepositoryTests.java

@ddixit14 ddixit14 requested a review from elefeint March 23, 2022 18:26
};
new Object[]{
"BUY", "abcd",
// this int param requires custom conversion
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put formatting back for this one statement? Otherwise it's unclear what number this comment refers to.

Comment on lines 864 to 871
void unSupportedOrTest() {

assertThatThrownBy(() -> queryWithMockResult("countByTraderIdOrPrice", null, getClass().getMethod("traderAndPrice")))
.hasMessage("Cloud Datastore only supports multiple filters combined with AND.");

queryWithMockResult("countByTraderIdOrPrice", null, getClass().getMethod("traderAndPrice"));

// this.partTreeDatastoreQuery = createQuery();
this.partTreeDatastoreQuery.execute(new Object[] {123L, 45L});
//this.partTreeDatastoreQuery.execute(new Object[] {123L, 45L});
Copy link
Contributor

Choose a reason for hiding this comment

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

This test was unfortunately structured in the first place with mock setup faling. Could you remove the commented-out lines, and add a comment to this test saying that "PartTreeDatastoreQuery constructor will fail as part of queryWithMockResult setup"?

The commented out line that was there in the first place shows why it was done this way -- getting all the setup in place before calling the constructor was too hard. This is commonly a sign that code under test has a structure that does not lend itself to testing.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@elefeint
Copy link
Contributor

The reason checkstyle complains about the file name not matching the top-level public class name is a minor false-positive error with checkstyle. It sees that there is an interface above the no-longer-public classname, and assumes the file name should match the interface name instead.

If you move the interface from being above the class to being below the class, you'll be able to remove the public keyword without running into problems with checkstyle.

If this was an important false-positive, we'd file an issue with checkstyle. But putting a repository interface first, above the class that's the main point of this file, was an odd choice anyway, so it's better to fix it in our repo.

@ddixit14 ddixit14 merged commit 95ed5b1 into main Mar 24, 2022
@ddixit14 ddixit14 deleted the datastore-test-mig branch March 24, 2022 19:16
kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this pull request Oct 20, 2022
* Migrating tests to JUnit5

* fixing code smell issues

* Fixing
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