-
Notifications
You must be signed in to change notification settings - Fork 2
RDM-4217 - Create Case Data with document attachment in perftest env #16
base: master
Are you sure you want to change the base?
Conversation
…g performance testing
…g performance testing
…g performance testing
S2S update (CNP)
src/test/resources/application.conf
Outdated
createCaseUrl = "caseworkers/6687/jurisdictions/PROBATE/case-types/GrantOfRepresentation/cases" | ||
} | ||
|
||
cnp_sprod { | ||
cnp_sandbox { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kapiljain786 should this be sandbox or perftest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed @hemantt. Sandbox and SPROD environment not exist any more so this configuration is not required. Removed SPROD and Sandbox config and added config for new perftest environment
src/test/resources/data/listofcases
Outdated
@@ -0,0 +1,31 @@ | |||
id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kapiljain786 these user-ids, isnt the file name misleading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not user-id and actually document ids. however listofcases.csv file is not required for CCD performance testing so removing this file from repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please hold on on merging I'd like to review this properly
…into CaseDocs # Conflicts: # gradle/wrapper/gradle-wrapper.jar # gradle/wrapper/gradle-wrapper.properties # src/test/resources/application.conf # src/test/resources/logback-test.xml # src/test/scala/uk/gov/hmcts/ccd/corecasedata/scenarios/GetCaseData.scala # src/test/scala/uk/gov/hmcts/ccd/corecasedata/scenarios/GetPaginationMetaData.scala # src/test/scala/uk/gov/hmcts/ccd/corecasedata/scenarios/GetUserProfile.scala # src/test/scala/uk/gov/hmcts/ccd/corecasedata/scenarios/PostCaseData.scala # src/test/scala/uk/gov/hmcts/ccd/simulation/CCDPTSimulation.scala # src/test/scala/uk/gov/hmcts/ccd/simulation/CCDSimulation.scala # src/test/scala/uk/gov/hmcts/ccd/util/CcdTokenGenerator.scala # src/test/scala/uk/gov/hmcts/ccd/util/PerformanceTestsConfig.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments
import scala.util.Random | ||
|
||
|
||
object CreateDIVCaseDataKJ extends PerformanceTestsConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename this object to CreateDIVCaseData without KJ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not needed under scenario folder so removed
|
||
// val EventId = "applyForGrant" | ||
private val rng: Random = new Random() | ||
private def d8MarriagePetitionerName(): String = rng.alphanumeric.take(10).mkString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have created this definition probably in PerformanceTestConfig or another abstract class CreateCaseData with a generic name and used it in the scenario like
("D8MarriagePetitionerName", getRandomGeneratedString())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed about this already for each case field we need to enter data with different length so would prefer to keep it like this.
import scala.util.Random | ||
|
||
|
||
object CreateSSCSCaseDataKJ extends PerformanceTestsConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KJ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not needed under scenario folder so removed
|
||
import scala.concurrent.duration._ | ||
|
||
object CrossCaseTypeAliasFuzzySearchOnText extends PerformanceTestsConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move all ES tests to another branch? Should not be part of this PR for create case data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes Moved Elastic Search, Cross Case type stuff in separate branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good only one point will like to raise is that, the create scripts, create case data in the initial state and the cases are not progressed to subsequent states. For more realistic prod like data, there should be substantial event history for the cases. Assuming this is a known fact when running the perf tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found multiple issues. High level of duplication. Misleading naming. New code lacks a proper structure. All this results in problems in extending and maintaining this in the future.
The comments I add are only some comments. I stopped reviewing after a while.
|
||
object GetPrintableDocumentsForEvent extends PerformanceTestsConfig { | ||
|
||
private val url: String = config.getString("caseDataUrl") + "/" + config.getString("getPrintableDocumentsForEvent") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename getPrintableDocumentsForEvent
to getCasePrintableDocuments
please. In other places in this file too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
def httpRequest() = { | ||
val s2sToken = CcdTokenGenerator.generateGatewayS2SToken() | ||
val userToken = CcdTokenGenerator.generateWebUserToken() | ||
http("TX09_CCD_GetPrintableDocumentsForEvent_getDocumentsForEvent") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the meaning of TX09 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's transaction name needed to identify timing for specific endpoint from gatling raw data
@@ -0,0 +1,2 @@ | |||
filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move these files into a subfolder tree called casesGenerator\documents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to keep test related static data files under resource folder. that's fine
|
||
import scala.concurrent.duration._ | ||
|
||
object GetPrintableDocumentsForEvent extends PerformanceTestsConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to GetCasePrintableDocuments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
def httpRequest() = { | ||
val s2sToken = CcdTokenGenerator.generateGatewayS2SToken() | ||
val userToken = CcdTokenGenerator.generateWebUserToken() | ||
http("TX07_CCD_SearchInputDetails_searchInputDetails") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we call this CCD_SearchInputDetails_searchInputDetails
. what is TX07 used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TX01, TX02.....TXxx all are transactions name.
val getSearchInputDetails = scenario("Search Input Details").during(TotalRunDuration minutes) { | ||
exec( | ||
httpRequest() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you format better here please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
println("create case url: " + CreateCaseUrl) | ||
println("create case token url: " + CreateCaseTokenUrl) | ||
|
||
private val url: String = config.getString("caseDataUrl") + "/" + config.getString("validateCaseDetails") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove :String
why using: config.getString("caseDataUrl") + "/"
? you can use caseDataUrl(....
rename url
to validateUrl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed url to validateUrl but other syntax looks fine
println("create case token url: " + CreateCaseTokenUrl) | ||
|
||
private val url: String = config.getString("caseDataUrl") + "/" + config.getString("validateCaseDetails") | ||
println("Retrieving validateCaseDetails URL : " + url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean? we don't retrieve urls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amended. it's just printing URL as output.
.pause(MinThinkTime seconds, MaxThinkTime seconds) | ||
} | ||
|
||
val waitForNextIteration = pace(MinWaitForNextIteration seconds, MaxWaitForNextIteration seconds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this var used anywhere? can't find any usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we need pace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this line.
|
||
import scala.concurrent.duration._ | ||
|
||
object ValidateCaseDetails extends PerformanceTestsConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All naming in this class is completely messed up. You're not Validating a case here, you are creating a case.
Class name wrong, urls names misleading
Please review carefully and fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed and all looks OK to me
that's validateCaseDetails test and working perfectly fine.
Object name ValidateCaseDetails is also looking fine to me.
|
||
object WorkBasketInputDetails extends PerformanceTestsConfig { | ||
|
||
private val url: String = config.getString("caseDataUrl") + "/" + config.getString("workbasketInputDetails") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated code. Why don't you reuse use caseDataUrl(
method provided by PerformanceTestsConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not duplicate.
string concatenation to build URLs.
.header("ServiceAuthorization", s2sToken) | ||
.header("Authorization", userToken) | ||
.header("Content-Type","application/json") | ||
.check(status in (200)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove not needed spaces please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not clear about this. remove what ?
Amended println text and reformatted code.
exec( | ||
httpRequest() | ||
) | ||
.pause(MinThinkTime seconds, MaxThinkTime seconds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct formatting please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
object CreateDIVCaseData extends PerformanceTestsConfig { | ||
|
||
|
||
private val rng: Random = new Random() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi level of duplication found here. Searching for 1000000 + rng.nextInt(9999999 - 1000000) + 1
finds 5 instances. This means we could introduce some helper methods
and reuse those rather than duplicating.
Even better we should have all these random values generation code extracted into a dedicated class. And reused everywhere
prodSSCSPagination = "caseworkers/560966/jurisdictions/SSCS/case-types/Benefit/cases/pagination_metadata?case.caseReference=test123456" | ||
prodDIVORCED8caseReference = "aggregated/caseworkers/560966/jurisdictions/DIVORCE/case-types/DIVORCE/cases?page=1&case.D8caseReference=EZ12D81234" | ||
prodDIVORCED8caseReferencePagination = "caseworkers/560966/jurisdictions/DIVORCE/case-types/DIVORCE/cases/pagination_metadata?case.D8caseReference=EZ12D81234" | ||
createCaseDIVUrl = "caseworkers/81d4aa29-7ba2-4884-a5a4-e2a0211bfe7c/jurisdictions/:jurisdictions_reference/case-types/:casetype_reference/cases" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createCaseDIVUrl, createCaseSSCSUrl, createCaseCMCUrl... all duplicated. Needs sorting out
ESSearch = "searchCases" | ||
docStoreBashURL123 = "https://dm-store-perftest.service.core-compute-perftest.internal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docStoreBashURL123? what does that means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docStoreBashURL123 not needed. removed
docStoreBashURL123 = "https://dm-store-perftest.service.core-compute-perftest.internal" | ||
docStoreBashURL = "https://gateway-ccd.perftest.platform.hmcts.net" | ||
prodSSCSWorkBasket = "aggregated/caseworkers/560966/jurisdictions/SSCS/case-types/Benefit/cases?view=WORKBASKET&page=1&case.caseReference=test1234567" | ||
prodSSCSPagination = "caseworkers/560966/jurisdictions/SSCS/case-types/Benefit/cases/pagination_metadata?case.caseReference=test123456" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
560966
used in multiple places. Duplication problem. Perhaps extract in a variable and reuse it?
PLEASE DO NOT MERGE THIS PR to Master.
This PR is still using Maven and Master using Gradle so I don't want to merge this PR into master.
Please just review this PR so can use this branch to complete data alignment in preftest environment to match PROD data volume.
Before creating a pull request make sure that:
Please remove this line and everything above and fill the following sections:
JIRA link (if applicable)
https://tools.hmcts.net/jira/browse/RDM-4217
Change description
Align perftest environment CCD data to Prod
Does this PR introduce a breaking change? (check one with "x")