Skip to content

Update NUnit3TestAdapter version to 4.0.0-beta.2#21284

Closed
Mohit-Chakraborty wants to merge 3 commits intomainfrom
mohitc/UpdateNUnitAdapterVersion
Closed

Update NUnit3TestAdapter version to 4.0.0-beta.2#21284
Mohit-Chakraborty wants to merge 3 commits intomainfrom
mohitc/UpdateNUnitAdapterVersion

Conversation

@Mohit-Chakraborty
Copy link
Copy Markdown
Contributor

No description provided.

@Mohit-Chakraborty
Copy link
Copy Markdown
Contributor Author

/azp run net - search - tests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Mohit-Chakraborty
Copy link
Copy Markdown
Contributor Author

/azp run net - search - tests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Mohit-Chakraborty
Copy link
Copy Markdown
Contributor Author

/azp run net - search - tests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@Mohit-Chakraborty Mohit-Chakraborty marked this pull request as ready for review May 25, 2021 02:40
@Mohit-Chakraborty
Copy link
Copy Markdown
Contributor Author

@mikeharder - I ran the live search tests and they passed. I think the change to update the NUnit test adapter has helped.
What other tests should I run here?

@mikeharder
Copy link
Copy Markdown
Member

/azp run net - core - tests

@mikeharder
Copy link
Copy Markdown
Member

/azp run net - storage - tests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Copy Markdown
Member

@Mohit-Chakraborty: I triggered runs of core and storage tests, if these pass I think this is safe enough to merge. If we find any failures after merge we can investigate.

@pakrym
Copy link
Copy Markdown
Contributor

pakrym commented May 25, 2021

Any particular reason to get the update especially a beta? We should compare test counts before/after.

@mikeharder
Copy link
Copy Markdown
Member

@pakrym: The latest RTM version is crashing with OOM per this issue: #20509. I don't think @Mohit-Chakraborty has been able to repro locally, so we're not sure exactly what's causing it. But I suspect it's an issue in NUnit3TestAdapter, so I suggested we try the latest version and it does seem to fix the OOM.

If you would rather not use a Beta, we could test GA versions older than 3.17.0 to see if any of them fix the OOM as well.

@pakrym
Copy link
Copy Markdown
Contributor

pakrym commented May 25, 2021

As far as I know, 4.0.0-* is a major rewrite of NUnit. I'm not strictly against it but think we should be careful taking it.

Doesn't Test host process crashed apply to the child testhost process?

@pakrym
Copy link
Copy Markdown
Contributor

pakrym commented May 25, 2021

Yep, use this test to repro the error message. This might be an actual test/product bug.

using System;
using System.Threading;
using NUnit.Framework;
using System.Collections.Generic;

namespace ooomunit
{
    public class Tests
    {
        [Test]
        public void Test1()
        {
            ThreadPool.QueueUserWorkItem(_ => 
            {
            List<byte[]> b = new();
            while (true)
            {
                b.Add(new byte[100000000]);
            }
            }
            );
            
            Thread.Sleep(1000000);
        }
    }
}

@pakrym
Copy link
Copy Markdown
Contributor

pakrym commented May 25, 2021

I would also look into the --blame-* set of parameters for dotnet test. https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-test

<PackageReference Update="NSubstitute" Version="3.1.0" />
<PackageReference Update="NUnit" Version="3.13.2" />
<PackageReference Update="NUnit3TestAdapter" Version="3.17.0" />
<PackageReference Update="NUnit3TestAdapter" Version="4.0.0-beta.2" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar to @pakrym's feedback I would like to avoid taking a dependency on a prerelease if it can be helped.

@mikeharder
Copy link
Copy Markdown
Member

I agree we need to step back and understand what is happening here. If this is indeed a test or product bug causing OOM, then why doesn't it repro with the newer NUnit3TestAdapter?

@pakrym
Copy link
Copy Markdown
Contributor

pakrym commented May 25, 2021

. If this is indeed a test or product bug causing OOM, then why doesn't it repro with the newer NUnit3TestAdapter?

Maybe some changes in test ordering/parallelization? I think adding an ability (or learning a way) to collect test crash dumps is a great exercise in general.

@Mohit-Chakraborty Mohit-Chakraborty deleted the mohitc/UpdateNUnitAdapterVersion branch July 2, 2021 18:57
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-net that referenced this pull request Oct 28, 2022
[Workloads - Sap Virtual Instance] (Azure#21284)

-Adds SAP software detection feature on a VIS
-Changes were already checked-in to private repo in the PR:
 Azure/azure-rest-api-specs-pr#8871
 The changes were verified. This commit just ports the same
 to the public repo.
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.

[BUG] Test runner process crashes due to OutOfMemoryException

4 participants