Mock Entrez PMIDlookup #112

tsande16 · 2024-05-09T18:08:29Z

The tests for EntrezPmidLookupTest were hitting against the real PubMed Entrez service. In most cases this would work, but if the service was down it would cause failures. This service has now been mocked in the tests.

To test:
mvn verify

rpoet-jh

Good job, Tim! I have a few comments.

rpoet-jh · 2024-05-10T20:29:45Z

...-transform-load/src/test/java/org/eclipse/pass/loader/nihms/entrez/EntrezPmidLookupTest.java

-    @Test
-    public void testGetEntrezRecordJson() {
-        PmidLookup apiService = new PmidLookup();
+    @MockBean


In order to use @MockBean, the test needs to have @SpringBootTest and maybe @TestPropertySource("classpath:test-application.properties"). With this, the test becomes spring aware, and spring will take care of wiring things up.

rpoet-jh · 2024-05-10T20:30:32Z

...-transform-load/src/test/java/org/eclipse/pass/loader/nihms/entrez/EntrezPmidLookupTest.java

-        String pmid = "29249144";
+    @BeforeEach
+    public void setUp() {
+        mockPmidLookup = mock(PmidLookup.class);


When you add @SpringBootTest, you will no longer need this method. Spring takes care of resetting the @MockBean between each test.

rpoet-jh · 2024-05-10T20:34:02Z

...-transform-load/src/test/java/org/eclipse/pass/loader/nihms/entrez/EntrezPmidLookupTest.java


-        JSONObject pmr = apiService.retrievePubMedRecordAsJson(pmid);
-        assertTrue(pmr.getString("source").contains("Proteome"));
+        when(mockPmidLookup.retrievePubMedRecordAsJson(pmid)).thenReturn(entrezJson);


From what I read, this test is just calling the method being mocked. This is not really testing the PmiLookup code.

I think a better approach would be to use wiremock, set the PmiLookup URL to hit that and return json. This way, the PmiLookup code is covered. See here for an example:

pass-support/pass-journal-loader/pass-journal-loader-nih/src/test/java/org/eclipse/pass/loader/journal/nih/DepositIT.java

Line 42 in b7d4e12

public class DepositIT {

I decided to go with putting the entrez service url in the application.properties and in the IT, it will be the same endpoint except I need to get the wiremock port real time in the IT. I was going make a setter method in PmidLookup class, does this sound like a good plan for you? I can also emulate what you did with making the entrez service url also a system property. I wasn't sure which would be the better way to go. On one hand it could be overkill having multiple ways to configure this URL (sys property and application.properties), but also using a setter method just for an IT doesn't feel right either.

I would try the following:

add httpPort to @WireMockTest to set the port, so something like @WireMockTest(httpPort = 9911)

set the entrez.url property to http://localhost:9911 in the test-application.properties

set the new PmidLookup.entrezUrl attribute using a @Value annotation

Note that I had to go with the Sys prop solution in journal loader because it is not spring boot enabled. Also note that using system props is part of standard spring boot property loading: https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#features.external-config . Not applicable for journal loader, but just an FYI.

Okay cool! I thought the port was randomly assigned, but if I can manual set it, all the better! Thanks for the info.

tsande16 · 2024-06-27T20:21:11Z

Going to wait until the release is complete and will do a rebase with the new version. Then this should be ready for review.

-Modified to use spring properties -Added JSON ascii file

rpoet-jh · 2024-07-08T17:45:54Z

pass-nihms-loader/nihms-data-transform-load/pom.xml

@@ -155,6 +162,38 @@
          </execution>
        </executions>
      </plugin>
+
+      <plugin>


This is not needed, it is declared in pass-nihms-loader/pom.xml.

rpoet-jh · 2024-07-08T17:56:59Z

Just one small change noted in #112 (comment) @tsande16 , then I will approve.

rpoet-jh · 2024-07-08T18:06:37Z

...-transform-load/src/test/java/org/eclipse/pass/loader/nihms/entrez/EntrezPmidLookupTest.java

-    public void testGetPubMedRecordWithHighAsciiChars() {
-        PmidLookup pmidLookup = new PmidLookup();
-        String pmid = "27648456";
+    public void testGetPubMedRecordWithHighAsciiChars() throws IOException {


A suggestion to reduce code is to update pmidrecord.json to have the high ascii character in the pmid_record_ascii.json title. Then you can just verify the title in testGetPubMedRecord and delete testGetPubMedRecordWithHighAsciiChars.

Other tests use this pmidrecord.json such as testNewCompliantPublication, testNewInProcessPublication, testNewNonCompliantPublication. Would it be okay to change these tests as well with the high ascii character?

These are in TransformAndLoadCompliantIT, TransformAndLoadInProcessIT, and TransformAndLoadNonCompliantIT

Done. This now includes the refactored NihmsSubmissionEtlITBase that has the member variables, title, doi, and issue that was used by the inheriting classes.

rpoet-jh · 2024-07-08T20:17:24Z

...-transform-load/src/test/java/org/eclipse/pass/loader/nihms/entrez/EntrezPmidLookupTest.java

+    @MockBean private NihmsTransformLoadCLIRunner nihmsTransformLoadCLIRunner;
+
+    @Value("${pmc.entrez.service.url}")
+    private String ENTREZ_PATH;


I think this can be deleted, not being used looks like.

rpoet-jh · 2024-07-08T20:17:36Z

pass-nihms-loader/nihms-data-transform-load/src/test/resources/pmid_record_ascii.json

@@ -0,0 +1,190 @@
+{


You can delete this file now.

rpoet-jh · 2024-07-08T20:18:49Z

...nihms-data-transform-load/src/main/java/org/eclipse/pass/loader/nihms/entrez/PmidLookup.java

@@ -54,8 +55,11 @@ public class PmidLookup {
     * delayed responses.
     * https://www.ncbi.nlm.nih.gov/books/NBK25497/
     */
-    private static final String DEFAULT_ENTREZ_PATH = "https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esummary" +
-                                                      ".fcgi?db=pubmed&retmode=json&rettype=abstract&id=%s";
+    /*private static final String DEFAULT_ENTREZ_PATH = "https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esummary" +


You can delete this too.

rpoet-jh · 2024-07-08T20:21:04Z

pass-nihms-loader/nihms-data-harvest/pom.xml

    <plugins>
      <plugin>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-maven-plugin</artifactId>
        <configuration>
          <mainClass>org.eclipse.pass.loader.nihms.NihmsHarvesterCLI</mainClass>
-          <finalName>nihms-data-harvest-cli</finalName>


I think this needs to remain here.

For this one I was getting compilation errors. I think there was a recent mvn change or something. I will put it back in to get the error message.

Done. This has been reverted, but will be looked into in the future, per our discussion.

tsande16 self-assigned this May 9, 2024

tsande16 requested a review from rpoet-jh May 10, 2024 13:47

rpoet-jh reviewed May 13, 2024

View reviewed changes

tsande16 added 12 commits July 8, 2024 09:40

Update test methods to mock PubMed entrez service

a28113e

Update testGetPubMedRecordWithHighAsciiChars

4e45c33

Modify newline char to System.lineSeparator()

63c19e9

Add pmc.entrez.service.url to properties

af4b0b2

Update Stubbing

cdd8be1

-Modified to use spring properties -Added JSON ascii file

Update dependencies

24d068e

Fix deps

c788731

Fix error with ascii character

83eecf1

Fix maven warning for finalName

53764c0

Fix javadoc

9285537

Add javadoc

35f496a

Add @param javadoc

a17245d

tsande16 force-pushed the 850-mock-entrez-it-pmidlookup branch from 8d81a2f to a17245d Compare July 8, 2024 14:09

tsande16 requested a review from rpoet-jh July 8, 2024 14:34

rpoet-jh reviewed Jul 8, 2024

View reviewed changes

Remove plugin

df22988

rpoet-jh reviewed Jul 8, 2024

View reviewed changes

Refactor NihmsSubmissionEtlITBase to include title, doi, and issue

a196970

rpoet-jh reviewed Jul 8, 2024

View reviewed changes

Cleanup

8084e37

rpoet-jh approved these changes Jul 8, 2024

View reviewed changes

tsande16 merged commit 043b435 into main Jul 9, 2024
2 checks passed

tsande16 linked an issue Jul 9, 2024 that may be closed by this pull request

Mock Entrez IT PMIDlookup eclipse-pass/main#850

Closed

tsande16 deleted the 850-mock-entrez-it-pmidlookup branch July 18, 2024 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mock Entrez PMIDlookup #112

Mock Entrez PMIDlookup #112

tsande16 commented May 9, 2024

rpoet-jh left a comment

rpoet-jh May 10, 2024

tsande16 Jun 27, 2024

rpoet-jh May 10, 2024

tsande16 Jun 27, 2024

rpoet-jh May 10, 2024

tsande16 May 14, 2024 •

edited

Loading

rpoet-jh May 14, 2024

tsande16 May 14, 2024

tsande16 Jun 27, 2024

tsande16 commented Jun 27, 2024

rpoet-jh Jul 8, 2024

tsande16 Jul 8, 2024

rpoet-jh commented Jul 8, 2024

rpoet-jh Jul 8, 2024

tsande16 Jul 8, 2024

tsande16 Jul 8, 2024

tsande16 Jul 8, 2024 •

edited

Loading

rpoet-jh Jul 8, 2024

tsande16 Jul 8, 2024

rpoet-jh Jul 8, 2024

tsande16 Jul 8, 2024

rpoet-jh Jul 8, 2024

tsande16 Jul 8, 2024

rpoet-jh Jul 8, 2024

tsande16 Jul 8, 2024 •

edited

Loading

tsande16 Jul 8, 2024

Mock Entrez PMIDlookup #112

Mock Entrez PMIDlookup #112

Conversation

tsande16 commented May 9, 2024

rpoet-jh 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

Choose a reason for hiding this comment

tsande16 May 14, 2024 • 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

tsande16 commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpoet-jh commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsande16 Jul 8, 2024 • 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

tsande16 Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsande16 May 14, 2024 •

edited

Loading

tsande16 Jul 8, 2024 •

edited

Loading

tsande16 Jul 8, 2024 •

edited

Loading