Skip to content

C#: Extensive use of stubs in testcases. #8279

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

Closed
wants to merge 31 commits into from

Conversation

michaelnebel
Copy link
Contributor

No description provided.

@michaelnebel
Copy link
Contributor Author

@hvitved : Before investing more time on this; Is this something we want?
(1) There might be a very small performance improvement (negligible) on test execution as the TRAP files will be slightly smaller by using a subset of the .NET Core stubs instead of the standard library as DLL's. However, loading the entire .NET core project as sources before test execution will worsen performance.
(2) This should be the real benefit; The test executions are based on stubs and not (maybe environment dependent?) DLL's.

What do you think?

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Some initial comments

@@ -1,5 +1,5 @@
import csharp

from Call call, Expr arg, string paramName
where arg = call.getArgumentForName(paramName)
where arg = call.getArgumentForName(paramName) and arg.fromSource()
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we treat everything in stubs as not fromSource already?

@@ -2775,7 +2775,7 @@ Assert.cs:
#-----| true -> access to parameter b2

# 140| [assertion failure] access to parameter b2
#-----| false -> [assertion failure] access to parameter b3
#-----| true -> [assertion failure] access to parameter b3
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is because the stubs are still based on .NET 5. When we upgraded to .NET 6, this line changed.

@@ -6600,10 +6600,20 @@ Finally.cs:

# 125| ... / ...
#-----| -> Double temp = ...
#-----| exception(DivideByZeroException) -> catch {...}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change System.Math.E on line 125 in Finally.cs to 2.7182818284590451. Or should we instead include constant values when generating stubs?

@@ -94,7 +94,6 @@ abstractValue
| non-null | Assert.cs:9:31:9:32 | "" |
| non-null | Assert.cs:10:9:10:13 | access to type Debug |
| non-null | Assert.cs:11:9:11:15 | access to type Console |
| non-null | Assert.cs:11:27:11:27 | access to local variable s |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be investigated

| non-null | Guards.cs:168:14:168:19 | access to type String |
| non-null | Guards.cs:169:13:169:19 | access to type Console |
| non-null | Guards.cs:169:31:169:31 | access to parameter x |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be investigated

Comment on lines -387 to -388
| non-null | Splitting.cs:107:13:107:13 | access to parameter o |
| non-null | Splitting.cs:109:13:109:13 | access to parameter o |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be investigated

| non-null | Splitting.cs:116:9:116:13 | access to type Debug |
| non-null | Splitting.cs:117:9:117:9 | access to parameter o |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be investigated

@michaelnebel michaelnebel deleted the csharp/use-stubs branch September 19, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants