Skip to content

Conversation

@ItsAdityaKSingh
Copy link
Collaborator

Partly fixes #46

Describe the changes you have made in this PR - Added test for landmark model

@Aadeesh11
Copy link
Contributor

Aadeesh11 commented Feb 4, 2022

Both LandMark and Location fromJson method is being tested in Beacon fromJson test. This can add redundancy. 😅

@ItsAdityaKSingh
Copy link
Collaborator Author

ItsAdityaKSingh commented Feb 4, 2022

There shouldn't be a redundancy as there are separate models for fetching it; having particular tests could increase code coverage and test each of them particularly too.

@nb9960
Copy link
Collaborator

nb9960 commented Feb 12, 2022

There shouldn't be a redundancy as there are separate models for fetching it; having particular tests could increase code coverage and test each of them particularly too.

But each field is already tested there too. I'd agreed to your statement if there would've other use cases for this model.
To reduce the redundancy what we can do is to just add validations here and PR #114 should use these. In any case, it would be better if you both collaborate and decide to go with one flow.

@ItsAdityaKSingh
Copy link
Collaborator Author

Sure @nb9960! I'll do the needful

@ItsAdityaKSingh
Copy link
Collaborator Author

Not necessary as PR #114 already handles it.

@ItsAdityaKSingh ItsAdityaKSingh deleted the landmark_model_tests branch March 2, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Tests]: Write Test for beacon project.

3 participants