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

Datarow tests - support methods with optional parameters and add params support #604

Merged
merged 7 commits into from
May 7, 2019

Conversation

ssa3512
Copy link
Contributor

@ssa3512 ssa3512 commented Apr 20, 2019

Resolves #252

@msftclas
Copy link

msftclas commented Apr 20, 2019

CLA assistant check
All CLA requirements met.

@jayaranigarg
Copy link
Member

@ssa3512 : Thank you for your contribution!!
PR looks good overall. Kindly do the needful changes so that we can push this in.

@jayaranigarg
Copy link
Member

@ssa3512 : It would be even more wonderful if you can address this related issue #283 as well as part of this change.

@jayaranigarg
Copy link
Member

@ssa3512 : Any updates on this? Let us know if you need some help on this.

@ssa3512
Copy link
Contributor Author

ssa3512 commented Apr 26, 2019

@jayaranigarg sorry, no updates yet. I'm hoping to have some time to address the feedback and look at including #283 early next week.

@ssa3512
Copy link
Contributor Author

ssa3512 commented Apr 27, 2019

@jayaranigarg I believe I addressed all feedback; should now resolve #283 as well. Please review again and let me know if anything else needs to be addressed.

@ssa3512 ssa3512 force-pushed the datarow-optional-params branch from b7700e6 to b77f624 Compare April 27, 2019 16:51
@jayaranigarg
Copy link
Member

@ssa3512 : Awesome work 👍 Left few minor comments, please address them and we should be all good to push this in.

…rived class tests and rename tests to be more descriptive
@ssa3512
Copy link
Contributor Author

ssa3512 commented May 1, 2019

@jayaranigarg if we remove the overridden tests from DerivedClass, the E2E tests run the base class tests twice - once for BaseClass and once for DerivedClass, causing the counts of passed tests to be doubled. Is it okay to specify FullyQualifiedName~BaseClass in the testCaseFilter for the new E2E tests to prevent this, causing only the base class to run, or should I break the new tests out into a separate DataRowTest class that does not have a derived class?

@jayaranigarg
Copy link
Member

@jayaranigarg if we remove the overridden tests from DerivedClass, the E2E tests run the base class tests twice - once for BaseClass and once for DerivedClass, causing the counts of passed tests to be doubled. Is it okay to specify FullyQualifiedName~BaseClass in the testCaseFilter for the new E2E tests to prevent this, causing only the base class to run, or should I break the new tests out into a separate DataRowTest class that does not have a derived class?

@ssa3512 : Oh I see. I believe if we just keep these tests in the derived class, then they should not be executed twice.

@ssa3512
Copy link
Contributor Author

ssa3512 commented May 2, 2019

@ssa3512 : Oh I see. I believe if we just keep these tests in the derived class, then they should not be executed twice.

@jayaranigarg In that case is everything good to go?

@jayaranigarg
Copy link
Member

@ssa3512 : I have updated my comments. You can remove the tests from the base class and have them in derived class only. It will be good to go after that.

@ssa3512
Copy link
Contributor Author

ssa3512 commented May 3, 2019

@jayaranigarg removed tests from base class and added the [DataRow()] tests that were only in base to Derived

@jayaranigarg
Copy link
Member

@ssa3512 : Looks nice🎉 I will go ahead and push this in. Thank you for the contributions 🥇

@jayaranigarg jayaranigarg merged commit 167533c into microsoft:master May 7, 2019
@jnyrup
Copy link
Contributor

jnyrup commented May 7, 2019

@jayaranigarg Should #283 be closed as well?

@jayaranigarg jayaranigarg changed the title Datarow tests - support methods with optional parameters Datarow tests - support methods with optional parameters and add params support May 7, 2019
@jayaranigarg
Copy link
Member

@jnyrup: Yes. This PR adds support for params as well. Updated the title to be more explicit.

@ssa3512 ssa3512 deleted the datarow-optional-params branch May 7, 2019 12:25
@julealgon
Copy link

julealgon commented Nov 5, 2019

params support is completely unrelated to supporting default parameter values.

I'd suggest updating the release notes to make it clear that params is also supported on 2.0.0 now. The current release notes doesn't mention it at all, I assume because it only keeps track of issues, and we didn't have an issue to track this part of the work here:

image

In the future, I'd suggest being mindful of these changes and:

  1. Sending them in separate PRs to promote isolation of changes
  2. Ensuring that all functional changes are covered by an issue item for tracking

EDIT: Looks like there was a specific issue to track the params part, but we forgot to close it in time to generate the release?

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.

DataRow does not work with optional arguments
5 participants