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

Add GitHub Actions for tests #31

Closed
wants to merge 16 commits into from
Closed

Add GitHub Actions for tests #31

wants to merge 16 commits into from

Conversation

rstm-sf
Copy link

@rstm-sf rstm-sf commented Oct 12, 2020

Hello!

Add GitHub Actions for tests

Can also close_ #33

@rstm-sf
Copy link
Author

rstm-sf commented Oct 14, 2020

* Fix runners tests for `net47` on VS (or separate PR)

This was partially solved.

It turned out that when running the test from studio BaseDirectory from AppDomain.CurrentDomain is equal to this path C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\Common7\IDE\Extensions\TestPlatform\

For CecilNetstandardTests and CecilTests this can be solved with something like

AppDomain.CurrentDomain.AssemblyResolve += (s, e) =>
{
    var name = e.Name.Split(',').First() + ".dll";
    return Assembly.LoadFile(Path.Combine(selfDir, name));
};

But XamlParserTests depend on this

public CompilerTestBase() : this(new SreTypeSystem())

or this

foreach (var asm in AppDomain.CurrentDomain.GetAssemblies())

which ultimately raises an exception like this

throw new XamlTypeSystemException("Unable to resolve type " + type);

@rstm-sf
Copy link
Author

rstm-sf commented Oct 16, 2020

* Fix tests

This actually works too, but dotnet test doesn't like it https://github.com/rstm-sf/XamlX/pull/3/checks#step:9:22

But it doesn't really work anywhere. I think need to think about it

or this

foreach (var asm in AppDomain.CurrentDomain.GetAssemblies())

@rstm-sf rstm-sf marked this pull request as ready for review October 17, 2020 14:47
@rstm-sf
Copy link
Author

rstm-sf commented Oct 17, 2020

This actually works too, but dotnet test doesn't like it https://github.com/rstm-sf/XamlX/pull/3/checks#step:9:22

#34

I believe adding the github actions is done. It looks like they are disabled here

@@ -135,11 +142,6 @@ public void TypeDescriptor_Stubs_Are_Somewhat_Usable()
return "Value";
}, null);
}

public static void SetAttachedProperty(ServiceProviderTestsClass target, string value)
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Author

Choose a reason for hiding this comment

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

This is necessary so that there are no unnecessary messages during build. Since xunit thinks this is a test. If you mark this as private, the tests will stop passing. Therefore, I separated it to another class. It looks better to me -- less dependencies

Warning xUnit1013 Public method 'SetAttachedProperty' on test class 'ServiceProviderTests' should be marked as a Theory.


#if NETFRAMEWORK
// TODO: It's the hack for VS tests
var selfDirectory = Path.GetDirectoryName(typeof(CompilerTestBase).Assembly.Location);
Copy link
Owner

Choose a reason for hiding this comment

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

Can we check that we are running for Windows and using VS test platform? Or is it OK to do with dotnet test + Mono on Linux too?

Copy link
Author

Choose a reason for hiding this comment

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

Can we check that we are running for Windows and using VS test platform?

I did not consider this, as it seemed that it would be easier to limit the .NET Framework. In addition, in my opinion, this does not really affect, except that VS for some reason changes AppDomain.CurrentDomain.BaseDirectory

Or is it OK to do with dotnet test + Mono on Linux too?

I think not, besides mono/mono#6984

But I am far from using mono

Copy link
Author

Choose a reason for hiding this comment

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

Now I have done for everyone, but with verification AppDomain.CurrentDomain.BaseDirectory

var selfDirectory = Path.GetDirectoryName(typeof(CompilerTestBase).Assembly.Location);
AppDomain.CurrentDomain.AssemblyResolve += (sender, args) =>
{
var name = args.Name.Split(',').First() + ".dll";
Copy link
Owner

Choose a reason for hiding this comment

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

You do realize that .First() can be replaced with indexer access, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but it seems to me that it will be more readable, and besides, it will still happen

Copy link
Author

Choose a reason for hiding this comment

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

Fix to indexer access

@rstm-sf rstm-sf mentioned this pull request Oct 18, 2020
Comment on lines +27 to +43
- name: Build CecilNetstandardTests
run: dotnet build tests/CecilNetstandardTests/CecilNetstandardTests.csproj
- name: Build CecilTests
run: dotnet build tests/CecilTests/CecilTests.csproj

- name: Test for netcoreapp3.1
run: dotnet test --framework netcoreapp3.1

- name: Test for net47 CecilNetstandardTests
if: matrix.os == 'windows-latest'
run: dotnet test --framework net47 tests/CecilNetstandardTests/CecilNetstandardTests.csproj
- name: Test for net47 CecilTests
if: matrix.os == 'windows-latest'
run: dotnet test --framework net47 tests/CecilTests/CecilTests.csproj
- name: Test for net47 XamlParserTests
if: matrix.os == 'windows-latest'
run: dotnet test --framework net47 tests/XamlParserTests/XamlParserTests.csproj
Copy link

@worldbeater worldbeater Oct 19, 2020

Choose a reason for hiding this comment

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

See also #35 (comment)

Probably the following setup might just work, however needs testing:

- name: Build Unit Test Projects
  run: dotnet build tests/**/*Tests.csproj
- name: Run Unit Tests
  run: dotnet test --no-build
  working-directory: src

Copy-pasteing stuff usually isn't the best way to go.

@rstm-sf
Copy link
Author

rstm-sf commented Jul 13, 2023

@kekekeks hello, does it make sense to update it?

@rstm-sf rstm-sf closed this Sep 1, 2023
@rstm-sf rstm-sf deleted the infra/add_github_actions branch September 1, 2023 10:39
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.

3 participants