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

implemented conditional OuterLoopAttribute #7370

Merged
merged 6 commits into from
May 13, 2021

Conversation

pavelsavara
Copy link
Member

Fixes #7368

Use case: I'm adding test echo server to xharness, so that platforms which use xharness could be moved to innerloop.

@pavelsavara pavelsavara requested a review from riarenas May 11, 2021 14:46
@pavelsavara pavelsavara changed the title added conditions as optional parameters of OuterLoopAttribute implemented ConditionalOuterLoopAttribute May 11, 2021
@pavelsavara pavelsavara marked this pull request as ready for review May 11, 2021 14:50
@riarenas
Copy link
Member

Adding some people who are better versed in xunit to discuss whether this is the right way to go.

@ViktorHofer @alexperovich @akoeplinger

@riarenas riarenas removed their request for review May 11, 2021 14:57
@ViktorHofer
Copy link
Member

ViktorHofer commented May 11, 2021

Why can't you use two attributes instead? If I'm not mistaken, that should already be possible today:

[Outerloop]
[ConditionalFact(...)]

@pavelsavara
Copy link
Member Author

@ViktorHofer the use case is other way around. I want the test to not be in outerloop category for some platforms. So that it would run in inner loop.

@ViktorHofer ViktorHofer requested a review from safern May 11, 2021 15:43
@akoeplinger
Copy link
Member

Would it make sense to add a condition overload to the constructor of the main OuterLoop type instead, rather than making a new type?

@pavelsavara
Copy link
Member Author

It's surely possible. I actually implemented that first and then changed my mind. But I don't have strong preference.

@akoeplinger
Copy link
Member

Ok, I don't have a strong preference either way too :)

Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ViktorHofer
Copy link
Member

I would prefer to avoid the additional type, especially as its name is a bit confusing.

@pavelsavara pavelsavara changed the title implemented ConditionalOuterLoopAttribute implemented conditional OuterLoopAttribute May 12, 2021
@safern
Copy link
Member

safern commented May 12, 2021

I would prefer to avoid the additional type, especially as its name is a bit confusing.

Agreed. I think it would be better to just use the current OuterloopAttribute, just how we did with ActiveIssue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants