Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test cases for internship #156

Merged
merged 5 commits into from
Oct 12, 2020

Conversation

keanecjy
Copy link

@keanecjy keanecjy commented Oct 11, 2020

Create unit test for all classes in internship and make sample internship data for tests
Update Status util class

@keanecjy keanecjy added this to the v1.2b milestone Oct 11, 2020
@keanecjy keanecjy self-assigned this Oct 11, 2020
@codecov-io
Copy link

codecov-io commented Oct 11, 2020

Codecov Report

Merging #156 into master will increase coverage by 1.43%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #156      +/-   ##
============================================
+ Coverage     31.03%   32.47%   +1.43%     
- Complexity      418      462      +44     
============================================
  Files           167      169       +2     
  Lines          2990     3061      +71     
  Branches        338      353      +15     
============================================
+ Hits            928      994      +66     
  Misses         2015     2015              
- Partials         47       52       +5     
Impacted Files Coverage Δ Complexity Δ
...dress/logic/parser/util/ApplicationParserUtil.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/seedu/address/model/application/StatusDate.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...c/main/java/seedu/address/model/util/DateUtil.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...main/java/seedu/address/model/util/StatusUtil.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
.../seedu/address/model/wrapper/AlphaNumericWord.java 90.00% <ø> (+90.00%) 7.00 <0.00> (+7.00)
...va/seedu/address/model/wrapper/NonEmptyString.java 90.00% <ø> (+90.00%) 7.00 <0.00> (+7.00)
...va/seedu/address/model/wrapper/PositiveNumber.java 80.00% <ø> (+80.00%) 6.00 <0.00> (+6.00)
...torage/application/JsonAdaptedApplicationItem.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ss/logic/parser/edit/EditCommandParserWrapper.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...ddress/logic/commands/edit/EditProfileCommand.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 502028e...e07a182. Read the comment docs.

@keanecjy keanecjy linked an issue Oct 11, 2020 that may be closed by this pull request
# Conflicts:
#	src/main/java/seedu/address/storage/application/JsonAdaptedApplicationItem.java
Copy link

@ZoroarkDarkrai ZoroarkDarkrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

@keanecjy
Copy link
Author

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

@ZoroarkDarkrai
Copy link

ZoroarkDarkrai commented Oct 12, 2020

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

I mean the toString, equality, hashcode tests. Could use VALID_WAGE, VALID_PERIOD, etc? If it's a better practice to use "Sunday", "React Native", etc. it's fine then.

@keanecjy
Copy link
Author

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

I mean the toString, equality, hashcode tests. Could use VALID_WAGE, VALID_PERIOD, etc? If it's a better practice to use "Sunday", "React Native", etc. it's fine then.

Not too sure what's the correct which is the correct way, but I just thought that doing it this way by writing 2 strings that are different objects would be better since we will be comparing periods with different strings in the actual implementation too

@ZoroarkDarkrai
Copy link

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

I mean the toString, equality, hashcode tests. Could use VALID_WAGE, VALID_PERIOD, etc? If it's a better practice to use "Sunday", "React Native", etc. it's fine then.

Not too sure what's the correct which is the correct way, but I just thought that doing it this way by writing 2 strings that are different objects would be better since we will be comparing periods with different strings in the actual implementation too

Sure then! I just thought maybe using constants could make it easier to see, that creating an object with the same input will result to an equal one. Like 2 objects created with VALID_PERSON_ALICE are equals but not with one created with VALID_PERSON_BOB.
LGTM!

@orzymandias orzymandias modified the milestones: v1.2b, v1.2 Oct 12, 2020
@ZoroarkDarkrai ZoroarkDarkrai merged commit e761686 into AY2021S1-CS2103T-T15-4:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update status util
4 participants