-
Notifications
You must be signed in to change notification settings - Fork 6
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 some tests for the repository assets wrangling options #36
Conversation
This is done to ensure the best compatibility with the test cases to be included Signed-off-by: Akashdeep Dhar <[email protected]>
Rework some other elements as they are needed to be Signed-off-by: Akashdeep Dhar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have one query here, otherwise test case and the code seems okay to me!
standard.sbrcavbl = list(brcslist) | ||
return True, brcslist | ||
else: | ||
return False, "Cloned source namespace assets could not be found" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we have removed the try blocks and expectations area, was there any specific reason for it? The removal of the try-except block means that any exceptions occurring within this code will propagate up to the caller without being handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samyak-jn, yes and the caller is capable of handling the exception.
Adding exception handling in both places, i.e. the caller and the driver makes less sense and makes it difficult to reproduce tests related to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coolio, makes sense!
Add some tests for the repository assets wrangling options
Merge this pull request after merging #32, please
Fixes #37