Normalize error handling for DOI service#134
Conversation
markpatton
force-pushed
the
1223-book-doi
branch
from
6b1991b to
33925f6
Compare
markpatton
force-pushed
the
1223-book-doi
branch
2 times, most recently
from
8aa1361 to
cb7f12c
Compare
…multiple requests about the same DOI. Better handle character encoding of responses.
markpatton
force-pushed
the
1223-book-doi
branch
from
2f65bb2 to
b71c69a
Compare
| */ | ||
| @Autowired | ||
| public PassDoiServiceController(RefreshableElide refreshableElide) { | ||
| this.elideConnector = new ElideConnector(refreshableElide); |
There was a problem hiding this comment.
I would recommend using Spring to create and wire up these DOI classes.
You could add the @Service annotation to XrefDoiService and UnpaywallDoiService, and @Component to ExternalDoiServiceConnector and ElideConnector. Then just pass them all into this constructor and set.
By doing this, you could then use @MockitoBean in tests to mock as required, and the constructor below would not be needed.
markpatton
force-pushed
the
1223-book-doi
branch
from
48bdf32 to
bc3dac0
Compare
rpoet-jh
left a comment
rpoet-jh
left a comment
There was a problem hiding this comment.
Good job on the Spring IOC changes. I reviewed all the code after that, just a few more comments.
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class PassDoiServiceControllerTest { |
There was a problem hiding this comment.
Looks like some of these tests are actually covered in the IT in pass-core-main. For example, testGetXrefMetadata_ServiceError_404 looks to be covered with this test
Normally, Controller tests are ITs. I suggest looking over these tests and seeing if they are already covered in DoiServiceTest. There may be some tests that cannot be tested easily as an integration test, those can stay in this class.
There was a problem hiding this comment.
The problem is the permutations with the external services. The integration test can only really test the success path since it hits the external services. If you start mocking the external services, then its not really an integration test anymore. So I just created a separate unit test.
The unit test gives us a way to test the failure paths of the external service integrations like Sonarqube is demanding. There is some test redundancy between the two approaches, but it doesn't seem like a big deal. I will add some documentation to the tests so it is less confusing.
...-doi-service/src/test/java/org/eclipse/pass/doi/service/ExternalDoiServiceConnectorTest.java
Outdated
Show resolved
Hide resolved
pass-core-doi-service/src/main/java/org/eclipse/pass/doi/service/UnpaywallDoiService.java
Outdated
Show resolved
Hide resolved
pass-core-doi-service/src/main/java/org/eclipse/pass/doi/service/XrefDoiService.java
Outdated
Show resolved
Hide resolved
|
Made some updates. See what you think. |
Looks good. Only comment is I think you can remove the constructor |
|
@markpatton oh yeah, to clean up a bunch of the sq warnings, you can remove the |
@markpatton you may have missed this small comment. |
|
I thought I had all of them! I see they didn't trigger the failure mode. Cleaned up. I wish there was a way to get a list of all the issues on the command line. |
@markpatton did you push the commit? I don't see it. |
|
rpoet-jh
left a comment
rpoet-jh
left a comment
There was a problem hiding this comment.
One of the acceptance tests failed, I think there is some flakiness in that test, I saw it fail a couple days back too. Not caused by this PR, approved.
2 participants
Uh oh!
There was an error while loading. Please reload this page.