Add DOB field and associated documents & tests #156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AddressBookSystemTest is getting bloated and its' responsibility begins to become unclear. This violates SRP. Let's extract setup methods into new classes, so that the responsibility of AddressBookSystemTest is solely to provide helper methods for tests to use.
ClockRule only injects a fixed clock at the start of each test method. Some test methods do multiple tests i.e. execute multiple commands, which implies that the status bar's timing is always being verified with the same timing. This causes the testing of the status bar's timing to be insufficient as we are only effectively testing that the first test updates the status bar's timing correctly. Let's add a method in ClockRule that updates the injected clock to the current time, and let's teach DeleteCommandSystemTest to call this method whenever a test is executed.
AppStateAsserts contains test verification methods. As the responsibility of AddressBookSystemTest is to provide helper methods for tests to use, the test verification methods should belong to AddressBookSystemTest. Let's move the test verification methods from AppStateAsserts to AddressBookSystemTest. This makes AppStateAsserts redundant, let's delete it.
The browser panel is only updated if and only if the selected card in the person list panel is changed. It is unnecessarily tedious for test classes to call both methods separately to check that both the browser panel and the person list panel has changed. For e.g., DeleteCommandSystemTest has to call both getBrowserPanel().isUrlChanged() and getPersonListPanel().isSelectedPersonCardChanged(). Let's add helper methods in AddressBookSystemTest assertBrowserUrlAndSelectedCardChanged() and assertBrowserUrlAndSelectedCardUnchanged(). These methods will then call the relevant methods in BrowserPanelHandle and PersonListPanelHandle to perform the test verifications.
All tests will have to verify that the command box, result display box and model are correct. These verifications are repeated in DeleteCommandSystemTest. When we implement future tests, we foresee these verifications will be repeated again. This violates DRY principle. Let's extract these common checks as a helper method in AddressBookSystemTest, so that DeleteCommandSystemTest and future tests do not have to repeat the same code.
CommandBox's style is not verified in tests. This check should be included to ensure thoroughness of the test verification. Let’s add methods to verify that the CommandBox's style is as expected.
assertDeleteCommandSuccess(...) and assertCommandSuccess(...) achieve roughly the same functionality. This results in both methods having similar header comments, making it hard to distinguish the difference between the methods. Let's inline assertDeleteCommandSuccess(...) and extract parts of the code into a helper method.
assertSelectedCardChange() only verifies that the selected card is different from the card that was remembered previously. This test verification passes even though the incorrect card may be selected. Let’s teach assertSelectedCardChange() to accept a new parameter Index expectedSelectedCardIndex, so that this method is able to verify that the correct card has been selected.
This method verifies that the starting state of the application is correct through the use of assertions. The method name should be updated to assertApplicationStartingStateIsCorrect() to better reflect what it is doing. Let’s update the method name and the header comments accordingly.
There are problems with system tests. For e.g. the CommandBox's style isn't verified during the tests and the design of AddressBookSystemTest and AppStateAsserts are hard to understand. This means that our tests are not thorough enough and future developers may find it hard to understand the system tests. Let's resolve these issues.
Our coding standard for Java requires header comments for all classes, public methods, and all non-trivial private methods. This rule is not enforced by checkstyle. Let's teach Checkstyle to enforce the coding standard above by adding the rules JavadocMethod and JavadocType, which cover the checks on methods and classes respectively.
…ds [se-edu#379] Our coding standard for Java requires header comments for all classes, public methods, and all non-trivial private methods. This rule is not enforced by Checkstyle. Let's teach Checkstyle to enforce the coding standard above by adding the rules JavadocMethod and JavadocType, which cover the checks on methods and classes respectively.
This method returns a defensive copy of the model. This isn’t an exact copy of the model as the ModelManager’s constructor initialises the FilteredList to show all persons, even though the original model’s FilteredList may not be showing all persons. As there is no way to make a defensive copy of the model, let’s return the original model instead.
Test classes have to call WebViewUtil#waitUntilBrowserLoaded(BrowserPanelHandle) whenever a command that causes the browser to load a new page is executed. This causes code repetition when other test classes are implemented. Let’s shift this method call to executeCommand(String), so that this method only returns after the browser has loaded. Also, let’s enhance the concurrency check so that WebViewUtil#waitUntilBrowserLoaded(BrowserPanelHandle) does not have to manually reset the boolean concurrency variable.
When TestApp is initialised, the default browser url page is loaded. There is no wait to ensure that this page is loaded before assertApplicationStartingStateIsCorrect() is called. This causes a race condition: if the page has not been successfully loaded before assertApplicationStartingStateIsCorrect(), this assertion method will fail. Let's teach AddressBookSystemTest to wait for the browser to load before calling assertApplicationStartingStateIsCorrect().
Test classes have to update the application’s clock after each command has been executed. In DeleteCommandSystemTest, the update is called in both assertCommandSuccess(...) and assertCommandFailure(...). This leads to a lot of code repetition when other system tests are implemented, as they will have to update the clock in their own implementations of assertCommandSuccess(...) and assertCommandFailure(...). Let’s update executeCommand(String) to update the application’s clock immediately before a command is executed so that test classes do not have to update it.
There are some areas of code in system tests that have minor bugs and code issues. For e.g. AddressBookSystemTest's header comments is outdated and TestApp#getModel() is returning the wrong model. Let's fix these minor issues.
Helper methods in tests throw checked Exceptions. As tests are not expected to handle checked Exceptions, it is unnecessary for helper methods to throw checked Exceptions. Let's update these helper methods to throw unchecked Exceptions instead. If tests call methods in the main code base that throw checked exceptions, they do not have to wrap the code around a try-catch block to rethrow an unchecked exception. This is because it makes the code unnecessarily longer. The purpose of this PR is to stop the throwing of checked exceptions from propagating, causing the tests and helper methods to need to throw them as well.
…ns (se-edu#628) Helper methods in tests throw checked Exceptions. As tests are not expected to handle checked Exceptions, it is unnecessary for helper methods to throw checked Exceptions. Let's update these helper methods to throw unchecked Exceptions instead. If tests call methods in the main code base that throw checked exceptions, they do not have to wrap the code around a try-catch block to rethrow an unchecked exception. This is because it makes the code unnecessarily longer. The purpose of this PR is to stop the throwing of checked exceptions from propagating, causing the tests and helper methods to need to throw them as well.
assertCommandFailure(...) creates a defensive copy of the TestApp's Model. This is unnecessary as TestApp#getModel() returns a defensive copy of the TestApp's Model as well. Let's shorten the code in assertCommandFailure(...) by simply calling TestApp#getModel().
Tests in the 'guitests' packages, such as SelectCommandTest, merely concern themselves with the relevant GUI components and the model, and do not verify other parts of the application (e.g. storage, status bar, ...). They do not fully qualify as system tests, and we already have unit tests and integration tests for commands, thus making them redundant. Also, without any system tests in the codebase, we are unable to demonstrate to the students how one should be written. Let's convert SelectCommandTest to a system test.
Update About Us and Developer Guide
…aster updating and resolving merge conflicts
# Conflicts: # src/test/java/seedu/address/logic/commands/PartialFindCommandTest.java
# Conflicts: # src/test/java/seedu/address/logic/commands/PartialFindCommandTest.java
Update all the related doc, fix the test for pfind
Updated doc files
…aster Updating cos my fetch is a failure
Update the about us format
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.