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

[#1234] Move relevant test cases in ArgsParserTest to RepoSenseTest class #1530

Closed
wants to merge 3 commits into from

Conversation

HCY123902
Copy link
Contributor

Resolves #1234

Commit message:

Move the relevant test cases in ArgsParserTest to RepoSenseTest

The relevance of some test cases currently in ArgsParserTest 
is more on testing the behavior of methods whose main logic stays 
in the RepoSense class, such as the getRepoConfiguration method 
with LocationsCliArguments, rather than testing the behavior of 
ArgsParser.

Moving these test cases to RepoSenseTest will make the test cases
more organized. Additionally, when new methods are added to
the driver RepoSense class, there can be new test cases added.

Let's move the relevant test cases to a new RepoSenseTest class

@HCY123902 HCY123902 requested a review from a team April 9, 2021 06:03
@HCY123902 HCY123902 changed the title [ #1234] Move relevant test cases in ArgsParserTest to a new RepoSenseTest class [#1234] Move relevant test cases in ArgsParserTest to a new RepoSenseTest class Apr 9, 2021
@HCY123902 HCY123902 requested a review from a team April 10, 2021 09:08
Copy link
Contributor

@gerhean gerhean left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dcshzj dcshzj left a comment

Choose a reason for hiding this comment

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

I don't agree that we should move them to a separate class, but I leave it for the seniors to give their inputs.

Also, do assign the tasks to yourselves if you are intending to work on it, or if you have already submitted a PR. This makes the issue tracker easier to navigate.

@HCY123902 HCY123902 requested review from a team and removed request for a team April 11, 2021 08:19
@HCY123902 HCY123902 changed the title [#1234] Move relevant test cases in ArgsParserTest to a new RepoSenseTest class [#1234] Move relevant test cases in ArgsParserTest to RepoSenseTest class Apr 12, 2021
@0blivious
Copy link
Contributor

Judging by the name and the aspects to be tested, I feel we should still assign them to ArgParser class. @fzdy1914 @Tejas2805

Copy link
Contributor

@Tejas2805 Tejas2805 left a comment

Choose a reason for hiding this comment

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

A majority of the testing involves the ArgsParserClass, therefore don't think this migration is necessary. @fzdy1914 your opinion?

@fzdy1914
Copy link
Member

Agree that it is trivial to do so. I would like to better classify it as ArgsParserTest also.

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.

ArgsParserTest: Some tests should be moved to separate file, as they test logic in RepoSense driver class
6 participants