Watson-Personal-Assistant / AudioClientSampleCodeJava

Apache License 2.0
1 stars 5 forks source link

Esilky unit tests#378 #4

Closed esilky closed 6 years ago

esilky commented 6 years ago

This:

These tests can be executed from within Eclipse by using the 'Run As - JUnit Test' right-click menu command on any of the test classes or the 'Suite' classes (in the _junitTests folder).

<< ToDo - implement running the tests from a Maven command as a separate change >>

esilky commented 6 years ago

I have some changes for enabling the Maven project to run the tests, but I'm going to wait to create an issue and branch for it until these get approved/merged.

erezbi commented 6 years ago

@esilky I think once we merge the previous PRs, and you pull from master into this branch, then only the new changes should be visible here. Also, we create a branch for each task. So each PR should have little changes in it, and probably they will not conflict. You should work based on master in the different branches that you do, and not based on each of your branches, if possible. Does this make sense?

esilky commented 6 years ago

@erezbi - If I work from master, then it does not include the changes I made to-date. In some cases that should be fine - but if I've made changes to (say) logging (Log4J) that have not been accepted yet, but I want to use that Log4J functionality for logging of 'performance' I can't simply work from master.

erezbi commented 6 years ago

@esilky you are right. We usually are able to work based on master. You can work based on other branch, I think that if it accepted before the second one, then there will not be a conflict. But until the first one is accepted, the second will show more changes than really exist there.

esilky commented 6 years ago

@erezbi Okay, that makes sense. I was just a bit confused why all of these changes were showing up.

esilky commented 6 years ago

@erezbi @arivatibm - Is there anything needed from me to get this merged?

arivatibm commented 6 years ago

@esilky Please ask Ramesh to test this after you are done with the code fix

arivatibm commented 6 years ago

QA testing by Ramesh I've asked him to do it just now   Regards,   Ari Volcoff Research Staff Member Watson Assistant Solutions , IoT And Wearables IBM - Haifa Research Lab  
Mobile: +972-58-4168357 | Phone: +972-4-8281319 E-mail: ARIV@il.ibm.com LinkedIn: https://il.linkedin.com/in/avolcoff
    ----- Original message -----From: Ed Silky notifications@github.comTo: Watson-Personal-Assistant/AudioClientSampleCodeJava AudioClientSampleCodeJava@noreply.github.comCc: arivatibm ariv@il.ibm.com, Mention mention@noreply.github.comSubject: Re: [Watson-Personal-Assistant/AudioClientSampleCodeJava] Esilky unit tests#378 (#4)Date: Wed, Jun 6, 2018 9:26 PM  @erezbi @arivatibm - Is there anything needed from me to get this merged? —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
 

arivatibm commented 6 years ago

@esilky told Ramesh he needs to make some additional fixes @erezbi FYI

esilky commented 6 years ago

@rameshupadhyaper This is tested by:

esilky commented 6 years ago

@rameshupadhyaper please let @arivatibm know when you have finished your testing.

@arivatibm do you need to do anything to clear your review? I made the requested changes.

arivatibm commented 6 years ago

@esilky there are some merge conflicts - can you resolve them by merging the master into your feature branch?

esilky commented 6 years ago

@arivatibm @erezbi the latest master is merged in. There shouldn't be any conflicts.

erezbi commented 6 years ago

@esilky I see you made more commits. Did @rameshupadhyaper test the PR after this last commit (eb77668 above)?

esilky commented 6 years ago

These were part of the merge from master into the branch for this PR. @rameshupadhyaper have you tested?

rameshupadhyaper commented 6 years ago

@esilky @erezbi Tested in window machine. Working fine as per requirement.