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

Added spock test cases for Repose #968

Closed
wants to merge 1 commit into from
Closed

Added spock test cases for Repose #968

wants to merge 1 commit into from

Conversation

richarxt
Copy link
Collaborator

While upgrading repose, we found out that no test case exist for Repose and we decided to write it using spock framework.

@richarxt richarxt marked this pull request as draft April 22, 2024 15:37
@richarxt richarxt force-pushed the repose_cpock_test branch 4 times, most recently from ac8ce37 to 174a23f Compare April 23, 2024 15:15
@richarxt richarxt marked this pull request as ready for review April 23, 2024 15:20
@richarxt richarxt requested a review from zzantozz April 23, 2024 15:20
zzantozz
zzantozz reviewed Apr 23, 2024
View reviewed changes
spock-tests/pom.xml Outdated
<url>http://blueflood.io</url>

<dependencies>
<!-- Spock Framework -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to add groovy and spock to the entire project? That way we can write spock tests anywhere in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DOne

spock-tests/src/test/groovy/com/rackspacecloud/blueflood/repose/ReposeSpec.groovy Outdated
Comment on lines 46 to 51
@Shared
String tenantId = System.getenv("TENANT_ID")
@Shared
String authToken = System.getenv("AUTH_TOKEN")
String envToRun = System.getenv("ENV_TO_RUN")?.toLowerCase() ?: "stage"
static def defaultRateLimit = Integer.parseInt(System.getenv("RATE_LIMIT_COUNT") ?: "0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent. These four values are effectively user options. You have two set as @Shared, one static, and one just a normal test property. That makes a reader think they have different purposes or uses. It looks like they should all be @Shared and final.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DOne...defaultRateLimit need to be static because it is getting used in loop in where clause of test and it should be @shared and static to be accessible

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, for that matter, there's not much reason all of these couldn't be static and final. It would convey more information about their usage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@richarxt richarxt force-pushed the repose_cpock_test branch from 174a23f to 710629e Compare April 24, 2024 15:14
@richarxt richarxt requested a review from zzantozz April 24, 2024 16:37
@richarxt richarxt force-pushed the repose_cpock_test branch from 710629e to 34ad04d Compare April 24, 2024 16:41
zzantozz
zzantozz reviewed Apr 24, 2024
View reviewed changes
blueflood-spock-tests/pom.xml Outdated
Comment on lines 11 to 12
<artifactId>blueflood-spock-tests</artifactId>
<name>Blueflood Spock Tests</name>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, isn't this module for repose tests? We wouldn't put all possibly spock-based unit tests in this module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed to Repose

blueflood-spock-tests/pom.xml Outdated
Comment on lines 17 to 30
<dependency>
<groupId>org.spockframework</groupId>
<artifactId>spock-core</artifactId>
</dependency>

<dependency>
<groupId>org.spockframework</groupId>
<artifactId>spock-junit4</artifactId>
</dependency>

<dependency>
<groupId>org.apache.groovy</groupId>
<artifactId>groovy-all</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean to just add dependency management of these things at the top level. I meant add them as test dependencies of the entire project, so that spock tests can be written anywhere. That would have to include the groovy compiler plugin, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DOne

pom.xml Outdated
Comment on lines 121 to 127
<dependency>
<groupId>org.apache.groovy</groupId>
<artifactId>groovy-all</artifactId>
<version>4.0.16</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also be test scope, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DOne

blueflood-spock-tests/src/test/groovy/com/rackspacecloud/blueflood/repose/ReposeSpec.groovy Outdated
@Shared
static final def defaultRateLimit = Integer.parseInt(System.getenv("RATE_LIMIT_COUNT") ?: "0")

def httpClient = HttpClients.createDefault()
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this also @Shared and final?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DOne

spock-tests/src/test/groovy/com/rackspacecloud/blueflood/repose/ReposeSpec.groovy Outdated
Comment on lines 46 to 51
@Shared
String tenantId = System.getenv("TENANT_ID")
@Shared
String authToken = System.getenv("AUTH_TOKEN")
String envToRun = System.getenv("ENV_TO_RUN")?.toLowerCase() ?: "stage"
static def defaultRateLimit = Integer.parseInt(System.getenv("RATE_LIMIT_COUNT") ?: "0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, for that matter, there's not much reason all of these couldn't be static and final. It would convey more information about their usage.

zzantozz
zzantozz reviewed Apr 24, 2024
View reviewed changes
Copy link
Contributor

@zzantozz zzantozz left a comment

Choose a reason for hiding this comment

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

Looking better, but this needs a README in the new module explaining what it's for and how to use it. Possibly mention it in the top-level README, too.

@richarxt richarxt force-pushed the repose_cpock_test branch from 34ad04d to 8cd3089 Compare April 26, 2024 13:34
@richarxt
Added spock test cases for Repose
bc89c1b 
While upgrading repose, we found out that no test case exist for Repose and we decided to write it using spock framework.
@richarxt richarxt force-pushed the repose_cpock_test branch from 8cd3089 to bc89c1b Compare April 26, 2024 14:54
@richarxt
Copy link
Collaborator Author

richarxt commented May 3, 2024

These changes are moved to new project bluflood-repose-tests.

@richarxt richarxt closed this May 3, 2024
@richarxt richarxt deleted the repose_cpock_test branch May 3, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zzantozz zzantozz zzantozz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@richarxt @zzantozz