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

CollectionAttribute seems to be not respected #48

Closed
weltkante opened this issue May 21, 2020 · 7 comments
Closed

CollectionAttribute seems to be not respected #48

weltkante opened this issue May 21, 2020 · 7 comments

Comments

@weltkante
Copy link

weltkante commented May 21, 2020

WinForms has some tests that cannot be executed in parallel, the corresponding class has an attribute [Collection("Sequential")] which is supposed to put all test methods of that class in a collection and execute them in sequence (the name is probably irrelevant, as far as I understand its just grouping by collection name, but I didn't research this in detail).

/cc @hughbe @RussKie @AArnott

@AArnott
Copy link
Owner

AArnott commented May 21, 2020

Interesting. I didn't think that that attribute was the responsibility of this Library. I thought Xunit itself would take care of it. I'll look into it.

@weltkante
Copy link
Author

It might have been broken by one of the overrides WinFormsFact/Theory provide

@AArnott
Copy link
Owner

AArnott commented May 23, 2020

I did some quick research in how CollectionAttribute works and don't see any correlation to the test discovery system.

So I wrote a quick test:

[Collection("A")]
public class ConcurrentClass1Tests
{
    [WinFormsFact]
    public void ConcurrentA()
    {
        Thread.Sleep(10000);
    }
}

[Collection("A")]
public class ConcurrentClass2Tests
{
    [WinFormsFact]
    public void ConcurrentB()
    {
        Thread.Sleep(10000);
    }
}

Without the Collection attribute on the test classes, Test Explorer in VS configured to run tests in parallel will run these two tests in parallel. But with the Collection attribute, only one test runs at a time. So I can't repro the problem you're describing.

@weltkante
Copy link
Author

Thanks, I'll have another look and see if I can build a repro. One difference in scenario to your snippet: the test runs I was debugging in WinForms were on the same class.

@AArnott
Copy link
Owner

AArnott commented May 23, 2020

Thanks. I look forward to your repro.

One difference in scenario to your snippet: the test runs I was debugging in WinForms were on the same class.

That's not how CollectionAttribute works, at least not base don my reading of the docs.
Per my reading, tests within a class never run in parallel anyway (given default behavior anyway). The CollectionAttribute allows you to extend that non-concurrency across multiple classes, as in my example.

@AArnott
Copy link
Owner

AArnott commented May 23, 2020

FWIW I just tried moving the two methods into one class and removing the Collection attribute. The VS Test Explorer still ran them one at a time, which is consistent with the xunit doc I link to.

@weltkante
Copy link
Author

That's not how CollectionAttribute works, at least not base don my reading of the docs.
Per my reading, tests within a class never run in parallel anyway (given default behavior anyway).

Thanks for looking into it and clearing up how its supposed to work, looks what I was seeing was a red herring.

I was running tests from VS ("Debug test" command on a single test class) and when I did break into the debugger due to failed tests I was seeing all other methods having a thread as well. I could reproduce that but the test tasks already are on RanToCompletion so I assume you report success back to Xunit before you terminate the threads.

Since the test failures looked like they were comming from running tests in parallel I probably was jumping to the wrong conclusions, sorry. Can't reproduce the test failures anymore so I'm not sure what was causing them, but apparently not what I thought it was.

AArnott added a commit that referenced this issue Jan 5, 2023
Add -y switch to choco install
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

No branches or pull requests

2 participants