-
Notifications
You must be signed in to change notification settings - Fork 657
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
[watermarkstat] Add unit tests for watermarkstat show commands #1157
Conversation
Signed-off-by: Neetha John <nejo@microsoft.com>
Signed-off-by: Neetha John <nejo@microsoft.com>
Signed-off-by: Neetha John <nejo@microsoft.com>
Signed-off-by: Neetha John <nejo@microsoft.com>
Signed-off-by: Neetha John <nejo@microsoft.com>
This pull request introduces 1 alert when merging 0bccf33 into e1244a5 - view on LGTM.com new alerts:
|
Signed-off-by: Neetha John <nejo@microsoft.com>
This pull request introduces 1 alert when merging 7ec4c19 into e1244a5 - view on LGTM.com new alerts:
|
@@ -1,4 +1,4 @@ | |||
#!/usr/bin/env python | |||
#!/usr/bin/python |
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.
Why change this?
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.
Ran into this error in PR tests after #1151 was merged,
/usr/bin/env: 'python': No such file or directory
https://sonic-jenkins.westus2.cloudapp.azure.com/job/common/job/sonic-utilities-build-pr/2537/console
Had to change the path to fix it
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.
That's strange. #1151 shouldn't have had an impact on this. Also, all other executable scripts in the repo still use #!/usr/bin/env python
. I feel like this change shouldn't be necessary.
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.
Not sure how to fix the build failure. seems to be failing every time utilities wheel is built. But if I retain the docker and run the test again using 'python setup.py test', I don't see the issue
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.
It sounds like it may be a PATH difference with the sonic-utilities build job in sonic-build-tools. But I'm surprised no other tests are failing like this. You're executing the watermarkstat script explicitly in the tests. Are other unit tests doing this? If so, I would expect them to fail, also.
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 am not executing the command directly. I am using 'show' commands to invoke that script. The same format is used in many of the utilities tests which are invoking modules under 'scripts' path and some of them also use "/usr/bin/env python". I fail to understand what is special about this particular script
what is pending here? can we merge this? |
@lguohan: I find it odd that Neetha needed to modify the shebang in this file. I would like to understand this better, as it shouldn't be necessary. If you'd like, we can merge this as-is, and @neethajohn can continue to investigate offline. |
@lguohan, can I go ahead with the merge? |
I'm OK with merging this, but I'd like to understand further what changed to require this shebang to change. NJ> I will investigate offline |
@neethajohn Please create PR for 201911 |
@neethajohn, @lguohan: I believe I found the root cause of the shebang issue. I have opened a PR to fix here: #1233 |
- What I did
Added unit tests for watermarkstat show commands
- How to verify it