Skip to content

[tests] Add disabled NET6 availability attribute test - #14106

Merged
chamons merged 17 commits into
dotnet:mainfrom
chamons:attribute_cecil_test
Feb 11, 2022
Merged

[tests] Add disabled NET6 availability attribute test#14106
chamons merged 17 commits into
dotnet:mainfrom
chamons:attribute_cecil_test

Conversation

@chamons

@chamons chamons commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

This test is double disabled:

  • Completely disabled until mellite is re-run with a hacked platform assembly and default platform assembly
  • Once that's fixed, I'll enable the test, but it'll still be disabled on generated code (via HasCodegenAttribute) until the generator is also fixed.

But I wanted to get it in and reviewed sooner.

@chamons

chamons commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

A lot of the duplication in functions at the bottom is due to the fact that CustomAttributes is not on any Cecil base class from what I could tell, so I had to abuse type overloading to share code.

@chamons chamons added the not-notes-worthy Ignore for release notes label Feb 8, 2022

@mandel-macaque mandel-macaque left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, not blocking for my nits, please clean them a little :)

Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment on lines +71 to +76
// #if DEBUG
// const string Filter = "AppKit";
// if (!fullName.Contains (" " + Filter)) {
// return;
// }
// #endif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remote it? or uncomment?

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.

I think this is fine to keep until we are done with the test in general

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm planning on landing these just to uncomment out when debugging the mel and generator changes.

Comment on lines +82 to +85
// // iOS implies maccatalyst, but only for parent scope
// if (parentAvailability.Contains("ios") && !parentAvailability.Contains("maccatalyst")) {
// parentAvailability.Append("maccatalyst");
// }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same.

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.

same as above

Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment thread tests/cecil-tests/AttributeTest.cs Outdated
@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

Generator Diff (no change)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 105 tests passed.

Failed tests

  • monotouch-test/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Test run failed.
    Tests run: 2772 Passed: 2579 Inconclusive: 11 Failed: 1 Ignored: 192)

Pipeline on Agent XAMBOT-1094.BigSur'
Merge cf7eff1 into 4a7bd98

Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment thread tests/cecil-tests/AttributeTest.cs
@chamons

chamons commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

Added that second suggested test.

Comment thread tests/cecil-tests/AttributeTest.cs Outdated
if (attribute.ConstructorArguments.Count == 1 && attribute.ConstructorArguments[0].Type.Name == "String") {
string full = (string)attribute.ConstructorArguments[0].Value;
switch (full) {
case string s when full.StartsWith("ios", StringComparison.Ordinal):

@tj-devel709 tj-devel709 Feb 9, 2022

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.

Woah, I've never seen this before! Very handy

Comment thread tests/cecil-tests/AttributeTest.cs Outdated
@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

Generator Diff (no change)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 105 tests passed.

Failed tests

  • trimmode link/Mac Catalyst [dotnet]/Release [dotnet]: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)

Pipeline on Agent XAMBOT-1098.BigSur
Merge ee58472 into d5c3f2f

Comment thread tests/cecil-tests/AttributeTest.cs Outdated
Comment thread tests/cecil-tests/AttributeTest.cs
Comment thread tests/cecil-tests/AttributeTest.cs Outdated
}

// Unfortunate state we need to keep since I can't see to walk "up" from a
// MethodDefinition get_Foo or SetFoo to see it's container's properties

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.

Something like this should work:

var getter = ...;
var property = getter.DeclaringType.Properties.Find (v => v.Name == getter.Name.Substring (4));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems to work, at least enough for this PR. I'll fix it up with the attribute changes in follow up.

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

Generator Diff (no change)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 105 tests passed.

Failed tests

  • monotouch-test/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Test run failed.
    Tests run: 2772 Passed: 2578 Inconclusive: 11 Failed: 2 Ignored: 192)

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 60690ae into 478c1d2

@chamons

chamons commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

Test failure was good old https://github.com/xamarin/maccore/issues/2443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

not-notes-worthy Ignore for release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants