1311 Add Mock Tests to assist with developing/testing REST API's locally. #1315
Conversation
UmenR
requested review from
jo-elimu,
nya-elimu and
Keerthi4308
and removed request for
a team
There was a problem hiding this comment.
@UmenR This looks like a good way to simulate JSON response from the server, thank you 🙂
A suggestion: Instead of replacing the functional (live) REST API test, what if we leave those, and instead add integration tests as a separate layer?
That way, we will eventually have 4 types of test layers:
- Unit tests - JUnit (run during local development)
- Integration tests - JUnit/Mockito (run during local development)
- REST API tests - JUnit (run after deployment to test server)
- UI tests - Selenium (run after deployment to test server)
@nya-elimuai Cool! Sounds good, But I'm a bit confused about the first two points, Shouldn't we swap the first two points? As in for Unit tests we can Mock the external services(External APIs or data from DB) because typically unit tests shouldn't rely on external services/inputs and are focused more on the functionality of a particular block of code. I focused particularly on this because currently, for any new API that I add I will not be able to run any tests against them because the changes are not in the test env. Then for integration tests, we can use both, eg: For GET type requests we can fetch them from the test env, And then for POST type of requests we can use Mockito. I've mainly focused on the REST APIs in this case but I might be missing the bigger picture here. Also, one more question that I have is I currently don't understand how we have separated out the two layers, As in Unit tests and Integration tests. Could you clarify that as well? eg: what commands/procedures we are to run for both these cases. As far as I understand when we run |
|
@UmenR Thanks for clarifying. Actually, I think we are on the same track already, just using different terms for the same things. 😉 So let me clarify by re-phrasing to "regression testing": When I wrote "integration testing", I was mainly thinking about "mocked integration testing". So it would look something like this:
The mocked integration tests are running relatively quickly (within seconds), right? If so, it makes sense to alway include them as part of the developer's build process. And we shouldn't mock server request/response data as part of the regression tests, since the regression tests rely on more realistic (live) test data from an actual database. Maybe we could rename some of these concepts to make the separation more clear. The main idea so far has been to use "regression testing" for tests that verify that existing functionality still works on the test server. |
|
@nya-elimuai Thanks for clarifying!! Now I thinkk I understand what you meant by having a seperate layer 😄 , In that case, what I was thinking of was shifting the new tests(ones that use Mockito) to a new location under test maybe by adding a new dev folder(test.java.dev.ai) that follows the same structure as we have inside (test.java.ai), And then include that directory under the default OR we can create a separate profile only for the tests within the new folder this way we don't make changes to the existing profiles, What do you think? However, one issue that I have right now is that we would now have to write two tests[One using Mockito (for unit and integration) and the other that uses test env API(for regression)] which I think is inevitable in our case. |
@UmenR Yes, that sounds like a good refactoring. And maybe with numbering to clarify the order of each category of tests?:
From what I understand, the order of execution would not matter for the integration tests. Meaning that it's probably okay if both the smaller unit tests and the mocked integration tests are run within the same group (Maven profile). In the future, we might have to ensure that unit tests are executed before the integrations tests, but as of now I think they can all be categorized within the same test suite.
I understand that concern. Still, they would not be 100% identical because their underlying test data would be different, isn't that right? |
|
@nya-elimuai , So I've moved the new TestCase that uses Mockito to a separate location which is, https://github.com/elimu-ai/webapp/blob/e0ef6ed44d72e590d44010602407a1ee84ac1c3e/src/test/java/dev/ai/elimu/rest/LetterToAllophoneMappingsControllerTest.java. If the above looks alright I think I can revert all changes done to src/test/java/ai/elimu/rest/v2/crowdsource/LetterToAllophoneMappingsControllerTest.java. So that as before it will only contain the integration tests. Next up I've added the TestSuit which includes the TestCase that uses Mockito And a TestSuit Runner that is responsible for running the test suit. However, I'm not entirely sure if this is what you meant by renaming the test suits? For this to work I need to change the current profiles to exclude all except for the TestSuiteRunner So that we can configure whatever TestCases to be included in the TestSuit and have them execute according to the profile. This means that we would have to add seperate TestSuits and Runners per each group of tests I.E Integration, Unit, etc. |
|
@UmenR The branch is not compiling for me: |
There was a problem hiding this comment.
@UmenR Could you please rename the title of the GitHub issue for this PR?: #1311
"Improve test cases" is too broad for me to understand what exactly this code change is for.
See https://github.com/elimu-ai/wiki/blob/master/CONTRIBUTING.md#how-to-implement-and-submit-your-work
Keep each commit small so that diffs are easy to read and understand for people looking at your code in the future. A pull request should always be focused to doing one thing, for example fixing a bug or adding a new feature, but not a mixture. Avoid large commits and pull requests which attempt to do too much at once, as this makes code review difficult.
And maybe you should create one GitHub issue for refactoring and another for adding a mock test? Because this feels like too many changes at once.
|
@nya-elimuai Agreed! this PR is trying to tackle multiple issues in one go. let me change the issue and the code in this PR to focus on adding the mock tests only so that it addresses the immediate issue I faced with testing new REST API endpoints, I shall open a new issue for dealing with refactoring the exiting tests later.
I'll remove the irrelevant code first, and let you know, which should fix the issue. |
UmenR
changed the title
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <beans xmlns="http://www.springframework.org/schema/beans" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation=" | ||
| http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd"> | ||
|
|
||
| <import resource="file:src/main/webapp/WEB-INF/spring/applicationContext-jpa.xml"/> | ||
|
|
||
| <bean id="letterToAllophoneMappingsController" | ||
| class="ai.elimu.rest.v2.crowdsource.LetterToAllophoneMappingsController"/> | ||
| </beans> |
There was a problem hiding this comment.
Is including this file a requirement for enabling the Spring mock (MockMvcRequestBuilders) functionality?
| @@ -1,22 +1,21 @@ | |||
| package dev.ai.elimu.rest; | |||
| package ai.elimu; | |||
There was a problem hiding this comment.
This test should be placed in the same package as the class being tested (i.e. ai.elimu.rest.v2.content)?
| public class LetterToAllophoneMappingsControllerTest { | ||
|
|
||
| @Mock | ||
| public class LetterToAllophoneMappingsControllerTest extends RestControllerTest{ |
There was a problem hiding this comment.
The file being tested is LetterToAllophoneMappingsRestController, right? If so, this test should probably be renamed to LetterToAllophoneMappingsRestControllerTest.
There was a problem hiding this comment.
| public class LetterToAllophoneMappingsControllerTest extends RestControllerTest{ | |
| public class LetterToAllophoneMappingsControllerTest extends RestControllerTest { |
| private LetterDao letterDao; | ||
|
|
||
| @Autowired | ||
| LetterToAllophoneMappingsController letterToAllophoneMappingsController; |
There was a problem hiding this comment.
Replace with LetterToAllophoneMappingsRestController?
| List<LetterToAllophoneMapping> testList = new ArrayList<>(); | ||
| testList.add(letterToAllophoneMapping); | ||
| Mockito.when(letterToAllophoneMappingDao.readAllOrderedByUsage()).thenReturn(testList); | ||
| public void setup() throws JsonProcessingException {Allophone allophone = new Allophone(); |
| allophoneDao.create(allophone); | ||
|
|
||
| Letter letter = new Letter(); | ||
| letter.setDiacritic(true); |
There was a problem hiding this comment.
The letter 'c' is not a diacritic, so should be set to false. However, false is the default value, so this line can be deleted.
| testList.add(letterToAllophoneMapping); | ||
| Mockito.when(letterToAllophoneMappingDao.readAllOrderedByUsage()).thenReturn(testList); | ||
| public void setup() throws JsonProcessingException {Allophone allophone = new Allophone(); | ||
| allophone.setValueIpa("ɛɛ"); |
There was a problem hiding this comment.
If you would like to check some examples of existing Letter-to-Allophone mappings, see http://eng.elimu.ai/content/letter-to-allophone-mapping/list
|
Closing this PR due to inactivity. |
Hi, @nya-elimuai I have made the changes required to run a single unit test without the requirement of the test env REST API, Could you please check if the changes below work as expected in your machine as well,
I.e: If this unit test can be run on its own without having to start the server, And if this unit test is run along with others if we run
mvn clean verify -P regression-testing-restetc.This code needs to be refactored but I just wanted to verify if what we are expecting is achieved before doing further refactoring.