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

[CI][C#] Verification jobs for C# are currently failing #43058

Closed
raulcd opened this issue Jun 26, 2024 · 10 comments
Closed

[CI][C#] Verification jobs for C# are currently failing #43058

raulcd opened this issue Jun 26, 2024 · 10 comments

Comments

@raulcd
Copy link
Member

raulcd commented Jun 26, 2024

Describe the bug, including details regarding any error messages, version, and platform.

Tests are failing with several errors that seem to be related to (see the logs for more details):

 ----- Inner Stack Trace -----
   at Apache.Arrow.Tests.CDataSchemaPythonTest.PythonNet..ctor() in /Users/runner/work/crossbow/crossbow/arrow/csharp/test/Apache.Arrow.Tests/CDataInterfacePythonTests.cs:line 48
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
  Failed Apache.Arrow.Tests.CDataSchemaPythonTest.ImportSchema [1 ms]
  Error Message:
   Class fixture type 'Apache.Arrow.Tests.CDataSchemaPythonTest+PythonNet' threw in its constructor

Job failures:

The failures started when those commits were merged:
ff9921f...f904928
There are several bump versions from dependabot on those that involves C#.

Component(s)

C#, Continuous Integration

@raulcd
Copy link
Member Author

raulcd commented Jun 26, 2024

@CurtHagenlocher @adamreeve @eerhardt any idea what is happening? this is a blocker for 17.0.0

@raulcd raulcd added the Priority: Blocker Marks a blocker for the release label Jun 26, 2024
@raulcd raulcd added this to the 17.0.0 milestone Jun 26, 2024
@CurtHagenlocher
Copy link
Contributor

The inner exception seems to be "Xunit.SkipException : PYTHONNET_PYDLL not set; skipping C Data Interface tests". This would happen if the environment variable PYTHONNET_PYDLL was not set on the VM image and is independent of any changes to product code. Something must have changed in the VM configurations?

@adamreeve
Copy link
Contributor

adamreeve commented Jun 27, 2024

It looks like these tests are supposed to be skipped in the verification jobs:

bool inVerificationJob = Environment.GetEnvironmentVariable("TEST_CSHARP") == "1";

if (inCIJob && !inVerificationJob && !pythonSet)
{
throw new Exception("PYTHONNET_PYDLL not set; skipping C Data Interface tests.");

Maybe the TEST_CSHARP environment variable is no longer being set, or it's now set to something other than "1"?

@adamreeve
Copy link
Contributor

adamreeve commented Jun 27, 2024

Actually the logs show that these are throwing an Xunit.SkipException rather than an Exception so it seems like the check for inVerificationJob is correct, but the skip is being incorrectly treated as an error.

So I think this is probably due to the either #41842 or #41848

@adamreeve
Copy link
Contributor

I can reproduce this locally and the tests pass (are skipped properly) if I revert #41842

@adamreeve
Copy link
Contributor

I've created a bug for xunit: xunit/xunit#2965

@CurtHagenlocher
Copy link
Contributor

I guess I don't have a firm grasp on the semantics of TEST_CSHARP or GITHUB_ACTIONS. These tests should certainly be run for at least some of the CI/checkin tests but perhaps not for all of them?

In any event, the xunit change is clearly a bug.

@raulcd
Copy link
Member Author

raulcd commented Jun 27, 2024

I can reproduce this locally and the tests pass (are skipped properly) if I revert #41842

Should we revert this temporarily for the release and create a follow up issue to update once the downstream issue is fixed?

@adamreeve
Copy link
Contributor

Should we revert this temporarily for the release and create a follow up issue to update once the downstream issue is fixed?

Yes I think that makes sense, I've made #43074 to revert the update and #43076 for the follow up.

kou pushed a commit that referenced this issue Jun 28, 2024
### Rationale for this change

See #43058, this release broke compatibility with xunit.skippablefact and caused skipped tests to be treated as failures.

### What changes are included in this PR?

This reverts commit ef3d467.

### Are these changes tested?

Yes

### Are there any user-facing changes?

No
* GitHub Issue: #43058

Authored-by: Adam Reeve <adreeve@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou
Copy link
Member

kou commented Jun 28, 2024

Issue resolved by pull request 43074
#43074

@kou kou closed this as completed Jun 28, 2024
adamreeve added a commit to adamreeve/arrow that referenced this issue Jul 1, 2024
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Jul 9, 2024
…che#43074)

### Rationale for this change

See apache#43058, this release broke compatibility with xunit.skippablefact and caused skipped tests to be treated as failures.

### What changes are included in this PR?

This reverts commit ef3d467.

### Are these changes tested?

Yes

### Are there any user-facing changes?

No
* GitHub Issue: apache#43058

Authored-by: Adam Reeve <adreeve@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants