Repository navigation
PDBStatusTest might cause flakiness due to network calls #910
Description
The following functions in PDBStatus are called from PDBStatusTest.java:
1.public static Status getStatus(String pdbId) throws IOException
2.public static Status[] getStatus(String[] pdbIds) throws IOException
3.public static String getCurrent(String oldPdbId) throws IOException
But these function makes network calls, due to latency and network errors, these tests might fail sometimes, introducing flakiness. So, a better way to circumvent this issue is via mocking.
Example: testGetStatus() calls getStatus() in PDBStatus and a call is made to the following URL:
https://data.rcsb.org/rest/v1/holdings/status/1HHB
Assert.assertEquals(Status.REMOVED, PDBStatus.getStatus("1HHB"));
Considering the data returned from the call, the resulting JSON node can be marked as:
String jsonString = "{\"rcsb_repository_holdings_combined\":{\"id_code_replaced_by_latest\":\"4HHB\",\"status\":\"REMOVED\",\"status_code\":\"OBS\"},\"rcsb_repository_holdings_combined_entry_container_identifiers\":{\"entry_id\":\"1HHB\",\"rcsb_id\":\"1HHB\",\"update_id\":\"2020_48\"},\"rcsb_id\":\"1HHB\"}";
ObjectMapper mapper = new ObjectMapper();
JsonNode **actualObj** = mapper.readTree(jsonString);
The getStatus() function is:
/**
* Get the status of a PDB id.
*
* @param pdbId the id
* @return The status.
*/
public static Status getStatus(String pdbId) throws IOException {
URL url = new URL(String.format(STATUS_ENDPOINT, DEFAULT_RCSB_DATA_API_SERVER, pdbId.toUpperCase()));
ObjectMapper objectMapper = new ObjectMapper();
JsonNode node = objectMapper.readValue(url.openStream(), JsonNode.class);
return parseStatusRecord(node);
}
We can use power mockito dependency in POM.xml
<!-- https://mvnrepository.com/artifact/org.mockito/mockito-all -->
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-all</artifactId>
<version>2.0.2-beta</version>
<scope>test</scope>
</dependency>
A few ways to mock the various objects are:
ObjectMapper mockObjectMapper = Mockito.mock(ObjectMapper.class);
InputStream inr = Mockito.mock(InputStream.class);
when(mockObjectMapper.readValue(Mockito.any(InputStream.class), JsonNode.class)).thenReturn(**actualObj**);
when(mockObjectMapper.readValue(Mockito.mock(InputStream.class), JsonNode.class)).thenReturn(**actualObj**);
This can be better achieved by moving parseStatusRecord() logic from getStatus() so that only the output from the network call can be mocked.
Activity
Shilpa-Rani commentedon Dec 11, 2020
Some of the other tests that failed due to network call flakiness in the builds since October 1st are:
Biojava-core:
Biojava-structure:
Other Reasons - Caused by: java.net.ConnectException:
Biojava-integerationtest
Error: TestQuatSymmetryDetectorExamples.testLocal:270 Stoichiometry A12B12C12D12E12H2 not expected for BIO:3JC9:1Shilpa-Rani commentedon Dec 14, 2020
A few more tests that fail in biojava-structure because of Network Call are as follows:
Required Logs:
Shilpa-Rani commentedon Dec 14, 2020
A few more tests that fail due to Network Failure in biojava-core:
Required Logs:
Shilpa-Rani commentedon Dec 14, 2020
A few more tests that fail in biojava-integrationtest because of Network Call are as follows:
sbliven commentedon Jan 4, 2021
Thanks for the list of network calls, @Shilpa-Rani.
I think we need to maintain some integration tests that include network calls so that we can detect changes in upstream data sources. However, I agree it would be nice if the main travis CI check was isolated from this so that builds don't fail immediately if the network goes down.
Is it possible to set up two travis processes that run for each commit? So for example the non-integration tests should be required to pass before merging, while the integration tests can sometimes be ignored if needed? Although I'm not sure whether that's really better...
unshorn commentedon Feb 27, 2022
This pull request: #1022
fixes the issue by adding a Junit rule to intercept network errors during test execution and skips those tests.
aalhossary commentedon May 9, 2023
Is this issue still valid or we should close now?
What about #1022 ?
josemduarte commentedon May 17, 2023
I think we should close this. It is true that it is not ideal that there's a dependency on network in some tests. We should strive to have all tests that depend on network in integration-test module.
Note: since transitioning to github workflows some time ago, we run integration-tests only in pull requests but not in master: see
biojava/.github/workflows/master.yml
Line 36 in 88b51de