PRMT-3413 E2E testing - Unexpected Interaction ID and NHS number #21

joefong-nhs · 2023-06-13T15:51:37Z

Added a test for unrecognised Interaction ID from mhs inbound queue
Added a test for UK05 with unrecognised NHS number from mhs inbound queue
Added a test for COPC continue request with unrecognised conversation Id from mhs inbound queue
Added model class and builder for test messages
Added HealthCheck class which calls the health check endpoints of each service e.g. ehr-repo

…ot exist on queue

… test messages

…5 template file

…and the other for unknown NHS number

… sec, unify the way to generate trigger message in two tests

…obvious that it is for testing

…h unknown conversation id

…ystem printf

…st naming

AndyFlintNHS

Only small bits to change - the main logic with the parameterised test looks very elegant!

AndyFlintNHS · 2023-06-14T14:35:07Z

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java

+
+import java.util.UUID;
+
+public final class ContinueRequestMessage {


I would just have this as public class. Usually you'd only use final in a case where you want to be certain nobody will extend this class in future

AndyFlintNHS · 2023-06-14T14:35:43Z

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessageBuilder.java

+
+import java.util.UUID;
+
+public final class ContinueRequestMessageBuilder {


I would just have this as public class. Usually you'd only use final in a case where you want to be certain nobody will extend this class in future

AndyFlintNHS · 2023-06-14T14:35:54Z

src/test/java/uk/nhs/prm/deduction/e2e/models/EhrRequestMessageBuilder.java

+
+import java.util.UUID;
+
+public final class EhrRequestMessageBuilder {


I would just have this as public class. Usually you'd only use final in a case where you want to be certain nobody will extend this class in future

AndyFlintNHS · 2023-06-14T14:37:01Z

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java

+    private String sourceGpAsid;
+    private String destinationGpAsid;
+
+    public UUID getConversationId() {


Could we please move these getters below the constructor?

AndyFlintNHS · 2023-06-14T15:57:55Z

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java

+                .replaceAll("200000000149", destinationGpAsid);
+    }
+
+    public String conversationId() {


I'm wondering if it's worth us breaking the conversationId and messageId methods out into a static method on a utility class. This UUID as upper case string format is useful across more than just the continueRequestMessage class and rather than us duplicating a lot of code, having something like this might help:

public static String getUuidAsUpperCaseString(UUID uuid) { return uuid.toString().toUpperCase(); }

Which could be called in the code like this (using line 34 as an example):

.replaceAll("DBC31D30-F984-11ED-A4C4-956AA80C6B4E", getUuidAsUpperCaseString(conversationId))

Hi, I have added the static method getUuidAsUpperCaseString to TestUtils class and call it for conversation ids and message ids.
Later could you have a look and see if I did it right?
Because conversation id is a private field so I keep the getter methods, but now all those getter would call to our getUuidAsUpperCaseString :)

I would suggest you don't even need the 'conversationId()' method at all. conversationId IS a private field as you say, but you don't need to use getters to reference private variables in the scope of the class where they're defined.

Sorry I overlooked this comment...
For conversationId(), in the e2e test that is a part that we need the conversation id from the message (the test use the conversation id to find an expected message in outbound queue), so I have kept the getter method for this purpose.

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessageBuilder.java

…class)

…seString to TestUtils

MohammadIqbalAD-NHS · 2023-06-26T09:09:25Z

src/test/java/uk/nhs/prm/deduction/e2e/utility/TestUtils.java

@@ -161,4 +162,8 @@ public static LargeEhrTestFiles prepareTestFilesForLargeEhr(

        return new LargeEhrTestFiles(largeEhrCore, largeEhrFragment1, largeEhrFragment2, ehrRequest, continueRequest);
    }
+
+    public static String getUuidAsUpperCaseString(UUID uuid) {


I would probably capitalise 'UUID' in the method name.

Suggested change

public static String getUuidAsUpperCaseString(UUID uuid) {

public static String getUUIDAsUpperCaseString(UUID uuid) {

Thanks! addressed this comment

martin-nhs · 2023-06-28T23:06:21Z

src/test/java/uk/nhs/prm/deduction/e2e/utility/HealthCheck.java

+                    .with()
+                    .pollInterval(200, TimeUnit.MILLISECONDS)
+                    .until(() -> getRequest(healthCheckUrl).getStatusCode().is2xxSuccessful(), is(true));
+        } catch (Exception e) {


Can we please change this to exception?

Suggested change

} catch (Exception e) {

} catch (Exception exception) {

Thanks! Addressed this comment

Sorry @joefong-nhs - I was meaning to add here, can we be specific to what exception we can expect to be caught here? A tip from me is if you hold your cursor over any logic, usually it'll display a definition where it will give you a lovely breakdown of the exceptions!

For example,

try {} catch(IllegalArgumentException exception) {}

In Java, we try to be as explicit as possible! 😄

Thanks! I have changed it like this:
catch (HttpClientErrorException | ConditionTimeoutException exception)

…-e2e-tests/pull/21\#discussion_r1252154140

joefong-nhs added 16 commits June 13, 2023 12:18

[PRMT-3413] Add method to queue helper to check that a message does n…

0ff49b1

…ot exist on queue

[PRMT-3413] Add url of ehr out service to test config

b746f75

[PRMT-3413] Add asid code to the Gp2gp system class

c72be7f

[PRMT-3413] Add an EhrRequestMessage class and its builder for making…

0264513

… test messages

[PRMT-3413] Fixed the incoherent conversation ids in RCMR_IN010000UK0…

83d6646

…5 template file

[PRMT-3413] Added two E2E tests, one for unrecognised interaction ID …

73221e1

…and the other for unknown NHS number

[PRMT-3413] Allow health check to wait for 2xx response for at most 5…

e548cd8

… sec, unify the way to generate trigger message in two tests

[PRMT-3413] Change the invalid interactionId used in test to make it …

7266c1c

…obvious that it is for testing

[PRMT-3413] Add test for Erroneous message: COPC continue request wit…

a2972cd

…h unknown conversation id

[PRMT-3413] Bundled test cases into one ParameterizedTest

ebaa402

[PRMT-3413] Change healthcheck class to use LOGGER.info rather than s…

c349de9

…ystem printf

[PRMT-3413] Remove duplicated tests, minor fix on log wordings and te…

e8c1fae

…st naming

[PRMT-3413] Minor fix on wordings

a2f073b

[PRMT-3413] Move the list of known asid codes to a static final field

6e37ee4

[PRMT-3413] Amended a misleading log message

956dfa7

[PRMT-3413] Amend incorrect LOGGER setting in new class

3f7a68d

joefong-nhs requested review from martin-nhs, AndyFlintNHS and MohammadIqbalAD-NHS June 14, 2023 13:15

AndyFlintNHS requested changes Jun 15, 2023

View reviewed changes

joefong-nhs added 2 commits June 15, 2023 11:55

[PRMT-3413] Address comments (change some final class to just public …

f3cc475

…class)

[PRMT-3413] Address comments and add a static method getUuidAsUpperCa…

8f6eaf9

…seString to TestUtils

joefong-nhs force-pushed the PRMT-3413 branch from 83de774 to 8f6eaf9 Compare June 15, 2023 12:38

MohammadIqbalAD-NHS reviewed Jun 26, 2023

View reviewed changes

[PRMT-3413] Addressed PR comment (getUUIDAsUpperCaseString)

a34a5c9

martin-nhs reviewed Jun 28, 2023

View reviewed changes

joefong-nhs added 2 commits July 4, 2023 10:18

[PRMT-3413] Address PR comment

161bc2d

[PRMT-3413] Address PR comment https://github.com/nhsconnect/prm-repo…

ea8bd7c

…-e2e-tests/pull/21\#discussion_r1252154140

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRMT-3413 E2E testing - Unexpected Interaction ID and NHS number #21

PRMT-3413 E2E testing - Unexpected Interaction ID and NHS number #21

joefong-nhs commented Jun 13, 2023 •

edited

Loading

AndyFlintNHS left a comment

AndyFlintNHS Jun 14, 2023

AndyFlintNHS Jun 14, 2023

AndyFlintNHS Jun 14, 2023

AndyFlintNHS Jun 14, 2023

AndyFlintNHS Jun 14, 2023 •

edited

Loading

joefong-nhs Jun 15, 2023

AndyFlintNHS Jun 30, 2023

joefong-nhs Jul 4, 2023

MohammadIqbalAD-NHS Jun 26, 2023

joefong-nhs Jun 28, 2023

martin-nhs Jun 28, 2023

joefong-nhs Jul 4, 2023

martin-nhs Jul 4, 2023

joefong-nhs Jul 4, 2023


		import java.util.UUID;

		public final class ContinueRequestMessage {


		import java.util.UUID;

		public final class EhrRequestMessageBuilder {

	public static String getUuidAsUpperCaseString(UUID uuid) {
	public static String getUUIDAsUpperCaseString(UUID uuid) {

PRMT-3413 E2E testing - Unexpected Interaction ID and NHS number #21

Are you sure you want to change the base?

PRMT-3413 E2E testing - Unexpected Interaction ID and NHS number #21

Conversation

joefong-nhs commented Jun 13, 2023 • edited Loading

AndyFlintNHS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyFlintNHS Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joefong-nhs commented Jun 13, 2023 •

edited

Loading

AndyFlintNHS Jun 14, 2023 •

edited

Loading