Skip to content

Unit test for NSMNSE-37: removeClientInterface() function #50

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

Closed
wants to merge 6 commits into from

Conversation

ziyuguo716
Copy link

This PR is a fix from #39 (Wrong branch used)

@CosminNechifor
Copy link
Contributor

Hey @ziyuguo716 add the context to circleci so that the image gets published.

@ziyuguo716
Copy link
Author

ziyuguo716 commented Dec 17, 2020

Hey @ziyuguo716 add the context to circleci so that the image gets published.

Hi @CosminNechifor, not sure if this addresses your comment: @shanchunyang0919 created a go-test circleci job in this PR. So, we are waiting for his circleci job to get merged into the workflow for both of our unit tests

Copy link
Contributor

@CosminNechifor CosminNechifor left a comment

Choose a reason for hiding this comment

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

Why wasn't this merged is there a reason for keeping it open?

@ziyuguo716
Copy link
Author

Why wasn't this merged is there a reason for keeping it open?

This PR is a subtask of NSMNSE-18. @shanchunyang0919 's #38 is the other part of the unit testing for pkg/universal-cnf/config/composite.go. Not sure if it's necessary to merge all our tests into composite_test.go, considering my unit tests are now in a separate file removeClientInterface_test.go. What do you think @CosminNechifor @tiswanso ?

@tiswanso
Copy link
Contributor

Sorry for leaving this dangling @ziyuguo716. What's the consensus on this? If it's not already superseded by other merges please create this PR against the wwwin-github.cisco.com repo and we can merge it in with a quicker final approval.

@ziyuguo716
Copy link
Author

@ziyuguo716 ziyuguo716 closed this Feb 19, 2021
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.

5 participants