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] add hlc test on Windows using MSBuild #11733

Draft
wants to merge 5 commits into
base: development
Choose a base branch
from

Conversation

yuxiaomao
Copy link
Contributor

This PR add hlc test on Windows. It depends on the template that I just added to hashlink: HaxeFoundation/hashlink@af63dba

WARNING: I disabled manually all workflow except windows64-test on hl, just to get a fast result, it uses nightly haxe. Need to revert it before merge: run cd extra/github-actions && haxe build.hxml, then push .github/workflows/main.yml.

It fail at some unit tests for now, should probably fix them some day.

@Simn
Copy link
Member

Simn commented Jul 24, 2024

Good initiative!

@yuxiaomao
Copy link
Contributor Author

It's ready for review and we can rerun it if we need to check current haxe with hl/c on windows. I marked it as draft to prevent merge (until we decide to skip some test or fixed the bugs).

Currently the result is

results: SOME TESTS FAILURES (success: false)
unit.spec.TestReflect
  test: FAILURE ...............................................F.............................................
    line: 97, expected false
unit.TestInt64
  testShifts: FAILURE .......FFF
    line: 492, expected true
    line: 492, expected true
    line: 492, expected true
unit.issues.Issue10752
  test: FAILURE F...
    line: 9, expected 2 but it is 0
unit.issues.Issue6705
  test: FAILURE .............................F
    line: 64, expected 2 but it is 0
  test1: FAILURE .............................F
    line: 246, expected 2 but it is 0
  testButEverythingIsDynamic: FAILURE .............................F
    line: 113, expected 2 but it is 0

Additionally, I have tested on local with optimization disabled

results: SOME TESTS FAILURES (success: false)
unit.spec.TestReflect
  test: FAILURE ...............................................F.............................................
    line: 97, expected false
unit.issues.Issue6705
  test: FAILURE .............................F
    line: 64, expected 2 but it is 0
  test1: FAILURE .............................F
    line: 246, expected 2 but it is 0
  testButEverythingIsDynamic: FAILURE .............................F
    line: 113, expected 2 but it is 0

@Simn
Copy link
Member

Simn commented Jul 25, 2024

It's ready for review

What do you mean exactly? I would think that the next step would be to address the unit test failures one by one.

@yuxiaomao
Copy link
Contributor Author

Sorry, I wasn't clear. I mean the CI code here is complete for me (if we add fix to hlc, it's to hlc and not to the CI), it can be merged - after enable main.yml - if we decided to skip those failure and fix them another time.
I'll check those failure when I have time.

@yuxiaomao
Copy link
Contributor Author

yuxiaomao commented Aug 8, 2024

I have fixed some failures caused by Visual Studio's optimization (merging identical function) in the templates: HaxeFoundation/hashlink@9cf7efa

Now there is only int shift overflow problems ("caused by" MaxSpeed in C compiler optimization settings). Maybe I'll need to implement the shift somehow in hl2c and force compiler to keep the behavior?

results: SOME TESTS FAILURES (success: false)
unit.issues.Issue10752
  test: FAILURE F...
    line: 9, expected 2 but it is 0
unit.TestInt64
  testShifts: FAILURE .......FFF
    line: 377, expected true
    line: 378, expected true
    line: 379, expected true

@yuxiaomao
Copy link
Contributor Author

With the last commit, it does not fail locally with Visual Studio GUI/MSBuild on both 2019 Community and 2022 Community, strange.

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.

2 participants