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

Block explicit tests for 3.10 #471

Merged
merged 7 commits into from
Mar 4, 2018
Merged

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Mar 3, 2018

The rest of the 3.10 work for #47.

I used a new, non-category, non-trait, separate, hidden bool TestProperty. This is cleaner than re-using the display values we put in the Category or Traits collections; otherwise, we'll detect NUnit categories or properties named Explicit as the run state when they are not, and block them too.

The last commit is the minimum possible part of the refactor #457 to remove the otherwise necessary ugly hack. I wanted to go further and do the same thing to the Category property, but that just would be starting to redo the same work in #457. I still want to do #457 ASAP post-3.10.

@jnm2 jnm2 changed the title [wip] Block explicit tests for 3.10 Block explicit tests for 3.10 Mar 3, 2018
@OsirisTerje
Copy link
Member

This commit is very big. Is this a mix of code from multiple things? The implementation of the hidden property looks good, but there is a lot of other stuff here I don't really get, my bad. Can you explain for me what it is doing?
I might need to pull down this branch and see what is up here before I can properly review it.
I see the PR also reports it has conflicts with master now.

@jnm2 jnm2 force-pushed the jnm2/filter_without_explicit branch from f83ec50 to 36bf90e Compare March 3, 2018 20:05
@jnm2
Copy link
Contributor Author

jnm2 commented Mar 3, 2018

Rebased on master.

No problem! This is not a mix of code from other things. Besides the XML doc typo, everything here was essential to the approach I took. I wanted to use the list of commits to document why each change was necessary as far as I could see.

I am happy to talk about what I did or try alternatives. 😊 Explaining the entire diff at once might be hard for me. Is there a specific change in a specific commit that I can start with?

@OsirisTerje
Copy link
Member

LTGM

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 4, 2018

@OsirisTerje I added the documentation. Does that look clear enough for future readers?

/// Stores the information needed to initialize a <see cref="TestCase"/>
/// which can be inherited from an ancestor node during the conversion of test cases.
/// </summary>
public sealed class CachedTestCaseInfo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a good name for this is InheritableTestInfo?

@OsirisTerje
Copy link
Member

Good to go , thanks !

@OsirisTerje OsirisTerje merged commit 3b9e395 into master Mar 4, 2018
@jnm2 jnm2 deleted the jnm2/filter_without_explicit branch March 4, 2018 01:52
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.

2 participants