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

Test coverage not reliable because of RemoteExecutor bug #1376

Closed
antonfirsov opened this issue Oct 11, 2020 · 8 comments · Fixed by #1659
Closed

Test coverage not reliable because of RemoteExecutor bug #1376

antonfirsov opened this issue Oct 11, 2020 · 8 comments · Fixed by #1659
Labels
unit tests upstream-issue Issue depends on upstream dependency fix.

Comments

@antonfirsov
Copy link
Member

Some of our tests are using the RemoteExecutor tool from dotnet/arcade.

It looks like these tests are not reporting potential failures because of dotnet/arcade#6371. We should resolve this ASAP.

@antonfirsov antonfirsov added this to the 1.1.0 milestone Oct 11, 2020
@JimBobSquarePants
Copy link
Member

Switching to the Ubuntu build might be enough for now. That seems to work according to my tests

@JimBobSquarePants JimBobSquarePants added upstream-issue Issue depends on upstream dependency fix. and removed blocking labels Oct 16, 2020
@JimBobSquarePants
Copy link
Member

I've removed the blocking label as we have a workaround but I'm keeping the issue open so we can track it as it makes testing a little more difficult on Windows in some situations..

@antonfirsov
Copy link
Member Author

antonfirsov commented Oct 29, 2020

I've removed the blocking label

I see your point, but I believe that after all the SIMD refactors are done, it would be important to get a reliable RemoteExecutor run on Windows too before pushing out a new release. Doesn't have to be on CI, could be an ugly one-time workaround, but we probably want to avoid surprises, even if it would cost us waiting 1-2 more weeks.

I can have a look at this, after finishing the active edges stuff.

@JimBobSquarePants
Copy link
Member

Yeah... I've been doing all this manually so far by writing/testing SSE first then adding AVX. It's not been fun!

I can't seem to get any movement upstream to get a fix in either. I'm hoping after the .NET5 launch I'll see something.

@antonfirsov
Copy link
Member Author

antonfirsov commented May 22, 2021

Considering all the SIMD PR-s happening, I would block new releases until this is fixed.

Note adapting dotnet/arcade#7426 became a non-trivial question, because arcade dropped 2.1 support in dotnet/arcade#6891, and our tests still target 2.1.

The solution depends on our .NET Core 2.1 end-of-support strategy. Official support ends on August 21, 2021.

We can do 3 things:

  1. Hard cut 2.1 from both product and test code. I wouldn't do it before September, and we probably don't want to block the fix until that time.
  2. Cut 2.1 from tests, but keep product code building against 2.1 for a few more months
  3. Do some hack in msbuild, eg. build 2.1 target can refer to earlier version of RemoteExecutor

@JimBobSquarePants
Copy link
Member

Wow. They’re really doing their best to make an open source project as useless for the general public as possible aren’t they? Kinda defeats the purpose.

Does RemoteExecutor actually have any dependencies on other projects? Could we strip it out and maintain it separately?

What was that alternative library?

@antonfirsov
Copy link
Member Author

antonfirsov commented May 22, 2021

Opened dotnet/arcade#7435, let's see how they react.

What was that alternative library?

https://github.com/tmds/Tmds.ExecFunction

Initially I wanted to switch after addressing tmds/Tmds.ExecFunction#11, but then I realized I just don't have the time to carry out the whole change, fixing RemoteExecutor seemed to be a cheaper option.

@JimBobSquarePants
Copy link
Member

Fingers crossed that is accepted. It would save us a world of pain. Thanks for taking this on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unit tests upstream-issue Issue depends on upstream dependency fix.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants